Defining a physical model

[AndrewSazonov]

Here I propose to discuss how the user creates a model (or phase) object in EasyDiffraction. This object is expected to contain all the parameters needed to define a crystal or crystallographic phase.

[AndrewSazonov]

Creating an empty model

Currently, to create a phase it is necessary to import the EasyDiffraction Phase class, which can then be added to the job object via job.phases.append(). A suggestion is to implement the job.add_phase() method for the job object and hide the creation of the phase from the user.

Another suggestion is to use id instead of name as the unique identifier of the phase. This would better match the CIF syntax, e.g. _pd_phase_block.id.

⚙ Current

from easyDiffractionLib import Phase

job.phases.append(Phase(name='lbco'))

💡 Suggested

job.add_phase(id='lbco')

[rozyczko]

Is id as descriptive as name, especially for users who are not very well familiar with the CIF structure? We could potentially use both for specifying the phase name in the constructor.

[damskii9992]

I would actually think id is less missleading, as name could suggest the name of the crystal phase, ie. zincblende or wurtzite or something similar. id is more clearly just an id for the specifically constructed phase and as such leads to less confusion.

[AndrewSazonov]

Setting basic parameters

Once a phase has been created and added to job, its basic parameters can be set in a way that resembles CIF syntax. However, there are a number of inconsistencies. It is suggested that the CIF vocabulary be repeated one-to-one where possible, omitting underscores at the beginning of names.

Examples of one-to-one correspondence:

CIF keys	EasyDiffraction names
_cell.length_a	cell.length_a
_space_group.IT_coordinate_system_code	space_group.IT_coordinate_system_code

Examples of minimally modified names:

CIF keys	EasyDiffraction names	Changes
_space_group.name_H-M_alt	space_group.name_HM_alt	- is omitted
_B_iso_or_equiv	b_iso_or_equiv	B becomes small b

CIF syntax

data_lbco

_space_group.name_H-M_alt "P m -3 m"
_cell.length_a 3.8908

⚙ Current

phase = job.phases['lbco']
phase.spacegroup.space_group_HM_name = "P m -3 m"
phase.cell.length_a = 3.8909

💡 Suggested

phase = job.phases['lbco']
phase.space_group.name_HM_alt = "P m -3 m"
phase.cell.length_a = 3.8909

[rozyczko]

We should probably lowercase all attributes to avoid confusion. Why name_HM_alt but b_iso_or_equiv?
We should be either fully CIF-compliant or just be consistent with the choice, like lowercase.
The only exception would be the hyphens (can't create variable names with hyphens), where we need to either convert - to _ or remove it, like you propose.

[damskii9992]

Personally, I don't like the idea of striving towards being CIF-compliant since it goes against our own coding standards, such as lowercase. But if the user-community is extremely familiar with CIF, more so than with programming in general, it might be more user-friendly for this community specifically.

[henrikjacobsenfys]

I think most users will either import a cif directly, use a GUI to add a phase, or copy/paste from a different project and change a few numbers manually. In other words, most people will not care about whether the syntax is identical to that from CIF or not. It makes sense to use the same general naming convention as CIF (there's no need to invent more names for the same thing), but sticking to our own standard is probably best.

[AndrewSazonov]

Right, it makes sense to follow the naming convention from CIF. Everyone has a different opinion on how to name a particular parameter. The beauty of following the CIF standards is that these names are chosen and maintained by multiple commissions of the International Union of Crystallography (IUCr). So I would generally have more confidence in the correctness of their choice than in any single person. Another advantage of CIFs is that their names are easily human readable and quite descriptive. So I would try to follow the CIF naming standards as closely as possible. But, as we can see, there are cases where we cannot be fully compatible with CIF. For example, in case the name contains hyphens (name_H-M_alt) or starts with digits (2theta_offset). In this case we just need to specify clear rules on how to convert from CIF to our standard. And since we can't be fully compatible with CIF anyway, I also suggest using all names in lower case. That way it aligns better with the PEP rules. But those are the only changes I would make. In this case both developers and users will be able to easily guess what a parameter name should be and what this parameter is needed for, just by looking directly at the IUCr web pages or by listing them on the EasyDiffraction documentation pages with links to the IUCr full descriptions.

[AndrewSazonov]

Setting tabular parameters

CIF allows table parameters to be set using the loop_ tag. Therefore, it is suggested to use the append method for a category object to reproduce the addition of a row to the table. The example below suggests using the append method for the category object atom_site of the phase, rather than adding the atom site directly to the phase using the add_atom() method.

Table

Parameters of the CIF category atom_site.

label	type_symbol	fract_x	fract_y	fract_z	occupancy	B_iso_or_equiv
La	La	0	0	0	0.5	0.6
Ba	Ba	0	0	0	0.5	0.1
Co	Co	0.5	0.5	0.5	1.0	0.1
O	O	0	0.5	0.5	1.0	1.3

CIF syntax

data_lbco

loop_
_atom_site.label
_atom_site.type_symbol
_atom_site.fract_x
_atom_site.fract_y
_atom_site.fract_z
_atom_site.occupancy
_atom_site.ADP_type
_atom_site.B_iso_or_equiv
La La 0.0 0.0 0.0 0.5 Biso 0.6
Ba Ba 0.0 0.0 0.0 0.5 Biso 0.1
Co Co 0.5 0.5 0.5 1.0 Biso 0.1
O  O  0.0 0.5 0.5 1.0 Biso 1.3

⚙ Current

from easyCrystallography.Components.AtomicDisplacement import AtomicDisplacement

phase = job.phases['lbco']
phase.add_atom(label="La", 
               type_symbol="La", 
               occupancy=0.5, 
               adp=AtomicDisplacement("Biso", Biso=0.6))
phase.add_atom(label="Ba", 
               type_symbol="Ba", 
               occupancy=0.5, 
               adp=AtomicDisplacement("Biso", Biso=0.1))
phase.add_atom(label="Co", 
               type_symbol="Co", 
               fract_x=0.5, 
               fract_y=0.5, 
               fract_z=0.5, 
               adp=AtomicDisplacement("Biso", Biso=0.1))
phase.add_atom(label="O", 
               type_symbol="O", 
               fract_x=0.0, 
               fract_y=0.5, 
               fract_z=0.5, 
               adp=AtomicDisplacement("Biso", Biso=1.3))

💡 Suggested

phase = job.phases['lbco']
phase.atom_site.append(label="La", 
                       type_symbol="La", 
                       occupancy=0.5, 
                       b_iso_or_equiv=0.6)
phase.atom_site.append(label="Ba", 
                       type_symbol="Ba", 
                       occupancy=0.5, 
                       b_iso_or_equiv=0.1)
phase.atom_site.append(label="Co", 
                       type_symbol="Co", 
                       fract_x=0.5, 
                       fract_y=0.5, 
                       fract_z=0.5, 
                       b_iso_or_equiv=0.1)
phase.atom_site.append(label="O", 
                       type_symbol="O", 
                       fract_x=0.0, 
                       fract_y=0.5, 
                       fract_z=0.5, 
                       b_iso_or_equiv=1.3)

[AndrewSazonov]

Creating a model from CIF

In addition to defining a phase programmatically, one need to be able to create a phase from a CIF string or file and add it to a job object.

CIF string

phase_cif = '''
data_coo

_space_group.name_H-M_alt "F m -3 m"

_cell.length_a 4.263
_cell.length_b 4.263
_cell.length_c 4.263
_cell.angle_alpha 90.0
_cell.angle_beta  90.0
_cell.angle_gamma 90.0

loop_
_atom_site.label
_atom_site.type_symbol
_atom_site.fract_x
_atom_site.fract_y
_atom_site.fract_z
_atom_site.occupancy
_atom_site.ADP_type
_atom_site.B_iso_or_equiv
Co Co  0.0 0.0 0.0  1.0  Biso 0.509
O  O   0.5 0.5 0.5  1.0  Biso 0.670
'''

⚙ Current

from easyDiffractionLib import Phases

phases = Phases.from_cif_string(phase_cif)
phase = phases['coo']
job.phases.append(phase)

💡 Suggested

job.add_phase_from_cif(phase_cif)

[rozyczko]

Should probably add support for both file and string read as separate calls

job.add_phase_from_file(cif_file)

job.add_phase_from_string(cif_string)

since cif is the default format, no need to add it to the method name

[damskii9992]

I don't have an issue with having a string read call, but I don't really see the potential use-case? If a user want to import a phase, won't they always have a CIF file to import? In fact, shouldn't we encourage users to have CIF structures defined as separate files and loading them in rather than defining them directly in a script?

[rozyczko]

For notebook users - sure. They will overwhelmingly just load an existing file.
Internally, we use the cif representation at various stages, so being able to convert a cif string into a Phase object is important.

[AndrewSazonov]

Showing phase identifiers

It is suggested to make the phase_names property available directly from the job object.

⚙ Current

print(job.phases.phase_names)

💡 Suggested

print(job.phase_ids)

[rozyczko]

And the standard __repr__ on Phases which prints out those names in the same manner?

[AndrewSazonov]

Right, I think this is needed for all EasyScience objects.

[AndrewSazonov]

Visualisation of crystal structure

Currently, the user needs to use an external library to plot crystal structures. It is suggested to add a simple method for the job object to hide this.

⚙ Current

import py3Dmol

structure_view = py3Dmol.view(linked=False)
structure_view.addModel(job.phases['lbco'].cif, 'cif')
structure_view.setStyle({'sphere':{'colorscheme':'Jmol','scale':.2},
                         'stick':{'colorscheme':'Jmol','radius': 0.1}})
structure_view.setBackgroundColor('#2b2b2b')
structure_view.addUnitCell()
structure_view.zoomTo()

💡 Suggested

job.show_crystal_structure(phase_id='lbco')

[damskii9992]

Hmm, I get the sentiment but I am actually not in favor of this as it clashes with the separation between lib and app. Having graphical functionality such as this could lead to problems with scripts running remotely without displays or without the proper graphical engines, never mind users wanting the possibility to personalize their structure view for presentation or articles.
And it also discourages users from using the app which has structure viewing built-in.
So I would instead have the "Current" method written in a guide in the documentation or elsewhere for those users who want the functionality.

[rozyczko]

Yes, this is a valid complaint. Separating functionality in an object is important.
However, we see multiple cases where such visualisation of internal data structures is common and widely used.
xarray, our own scipp, or even pandas.

Being able to work on a Job object and immediately see a chart, without having to import, initialize and set up matplotlib is likely to be very welcome by users.

[AndrewSazonov]

I think it is important to be able to display crystal structure or measured and calculated diffraction patterns in a simple predefined way with just one line of code. But it also annoys me if installing a crystallographic module causes the installation of a visualisation library, as is the case with crispy and its dependency on matplotlib. I don't know how this is done in the packages that @rozyczko mentioned, but I would suggest that the visualisation library should be among the optional dependencies when installing easydiffraction via PyPi. These optional dependencies can be listed in the pyproject.toml file. If the visualisation library was not explicitly mentioned when installing esydiffraction, then job.show_crystal_structure(phase_id='lbco') should simply tell the user that this library should be installed and how to do this. And, of course, the user is free to choose any other library for custom visualisation.

[rozyczko]

Removal of phase

Currently, phase is being removed on the interface. The new setup means that we should be able to remove phases on the Job instead.

⚙ Current

We need to pass both the Phases object and the Phase to be deleted.

interface.remove_phase(phases_obj=phases, phase_obj=phases[0])

💡 Suggested

job.phases.remove_phase(phase_id)

[AndrewSazonov]

Maybe just

job.remove_phase(phase_id)

to be consistent with job.add_phase?

[rozyczko]

Following the suggestions from the next comment, we should try to aim at having simple, generic actions on specific objects, like remove(), add(), append() etc. instead generalizing those to appropriate methods on specific objects.
This would mean

job.phases.remove(phase_id)

job.phases.append(phase)

[rozyczko]

Removal of atoms (and other tabular parameters)

Suggestion: mimic the current behaviour for clarity and ease of use rather than add the atom_site attribute as with adding an atom.

⚙ Current

phase.remove_atom(atom_label)

💡 Suggested

phase.remove_atom(atom_label)

# rather than
# phase.atom_site.remove_atom(atom_label)

The follow-up suggestion is to simplify the access to atoms and their properties directly from Phase, e.g.

phase.atom[atom_label].occupancy = 1.0

[rozyczko]

One potential problem I see with this approach is when the CIF standard changes. We know that CIF1 -> CIF2 change modified many names in the dictionary (e.g. _atom_site_adp_type vs _atom_site.adp_type) effectively breaking the "CIF API".

E.g. for space_group_name_H-M - we would presumably use

phase.symmetry.space_group_name_h_m

since in v2, the hierarchy is _symmetry.space_group_name_H-M.
In v1, this didn't contain a dot, so we would need to call

phase._symmetry_space_group_name_h_m('P1')

I am not sure that tying ourselves to a single version of the CIF standard is good. We already have to support both v1 and v2 keywords in our internal CIF parser.

Having aliases would make it easier to avoid breaking changes in case underlying standard changes.

Just something to think about. I'm fine with using 1-to-1 namespace between CIF dict and our method names, just thinking about potential pitfalls.

[AndrewSazonov]

While I disagree that "no users know CIF names anyways", it doesn't really matter whether or not the users know the CIF names. Already in this discussion, there are differing opinions among the three of us about what to name a particular object. It's quite common and understandable that each person may have a different opinion, that's fine. But are we sure that the names chosen based on our personal preferences are more correct and understandable for the whole community compared to the names chosen by the IUCr commissions working on the CIF dictionaries? Personally, I am not sure that the names I like are more correct and better describe these objects than the names based on CIF. That's why I suggest using CIF. Again, not because users are or are not familiar with CIF, but simply because these names have already been selected for us by experts in the field. So what's the point of redoing the work of the IUCr commissions?

And what's the point of having different names in different parts of the program instead of being consistent and using the same name everywhere, including frontend and source code? If we know that every name is CIF-standardized, then any time a user or developer finds a name whose meaning they don't know, they can look that name up in CIF dictionaries and find a detailed explanation of what it is. If we implement our own names, we need to define our own dictionary with all these descriptions. Again, what's the point of doing this extra work if it's all already done for us by the IUCr commissions?

I believe that having aliases will conversely make the code harder to work with. If there is one and only one way to do something, then everyone is forced to use that way. Remember Phyton's Zen: "There should be one-- and preferably only one --obvious way to do it"? If we have aliases, sooner or later we will end up with the same objects having different names in different parts of the code. The same thing will happen with users. In some Jupyter notebooks we'll use some names, like atom_sites, in other notebooks we'll just have aliases, like atoms, meaning the same thing, and the user will need to be told that it's the same thing. Then in the GUI we'll have something else, and it will need to be explained that it's actually the same as those two in the library. So what's the point of complicating life and confusing ourselves and users?

Now about the changes to the CIF standard and names with underscores vs. names with dots, like _atom_site_adp_type vs. _atom_site.adp_type. I don't see this as a name change, just a change in how a parameter name is separated from its category name. In both the old and new CIF, we have anatom_site category. This category contains a number of parameters, such as adp_type. In the old format we refer to this property as _atom_site_adp_type, and in the new format we refer to it as _atom_site.adp_type. I believe that we should not support v1 and v2 keywords in our internal CIF parser. Instead, I think we should only support v2 and convert v1 to v2 using whatever CIF library is available every time we load a CIF, and then we should internally only work with v2, i.e. with a point-based format.

Of course, the CIF standard may change over time. The current way of working with categories via points fits well with the Python way of accessing properties, and that's great. If CIF changes dramatically over time, then we'll see. But that could happen anywhere and with any other library. Also, the current CIF is already quite mature and I don't expect frequent changes.

[rozyczko]

How are we going to account for non-diffraction specific Phase descriptions?

In reflectometry you would have to create a completely new subset of CIF dictionary corresponding to existing ORSO standard, since reflectometry doesn't deal with atomistic models. In QENS, the Phase does not exist at all, since the model is just a set of Lorentzians.

If our Job is to contain Phase, Experiment etc. then we should either generalize Phase to be able to describe all kinds of atomistic or mesoscale materials OR have technique specific Phase, deriving from a base class, agnostic wrt. to the model.

Generalization means extending the existing CIF dictionary with a lot of new entries, created by us.
Deriving means we need to find a set of common data/methods used in each technique, applicable to Phase.

This boils down to a question posed elsewhere in the discussions:

• will the Job class derive from the technique independent JobBase or will we just create Job class differently in each Library?

Looking closer it seems that the Phase component might be the only thorny bit, since the rest (project, experiment, model, analysis, summary) should be (almost) the same.

So maybe only the Phase class implementation should be technique-specific? Then, using CIF namespace for diffraction, Bragg and spectroscopy would probably make sense.

[rozyczko]

(yes, I know this discussion is about the Phase class in Easy Diffraction only but we need to think about the whole EasyScience framework, since the Job concept is to be reused by all libraries)

[AndrewSazonov]

I agree that the Phase class implementation should be technique-specific. We should have a generic model container, which represents a phase in standard powder diffraction but could be something else in other techniques.

For powder diffraction, I strongly emphasize that most, if not all, phase properties are already standardized by IUCr commissions and documented in CIF dictionaries. Therefore, it is more straightforward to follow CIF naming conventions by default, rather than inventing yet another custom set of keywords. This avoids the unnecessary burden of teaching users an additional set of terms that they would still need to map to CIF standards.

For other techniques, where a non-diffraction Phase is needed, they should either:

• Use existing community-accepted standards for parameter naming, or
• Develop a new standardized set of keywords if one does not yet exist.

A good example is reflectometry, where the ORSO standard initiative exists. In this case, I believe it is beneficial for EasyReflectometry to strictly follow ORSO parameter naming, just as we adhere to CIF in diffraction.