restore reflections to the proper sample

[prjemian]

• close BUG: restore(clear=False) reflections added to other samples #289

[prjemian]

@strempfer, see if this branch resolves the problem you reported

[prjemian]

@padraic-shafer : Hope that 94fef13 is what you had in mind.

@mrakitin This is ready for review. @strempfer has been requested to test it (since it was he who reported the problem).

[prjemian]

off line conversation with @strempfer:

[9:15 PM] Strempfer, Joerg
... It looks like everything is read correctly when using the overwrite option (clear=True). But when using clear=False, it is also replacing everything and is not appending.

[9:18 PM] Jemian, Pete R.
Can you send me a copy of the config file you are restoring AND also make an export of the diffractometer just before you do the restore? That will help me to track this down.
[9:18 PM] Jemian, Pete R.
I expect the two files will not be identical. Right?

[9:32 PM] Strempfer, Joerg
sorry, I was just re-reading the file and expected the reflections to append, as was the case before, but which I think was already the bug. Now it is replacing only reflections which are identical and already there before the sate was saved when doing clear=False but leaved new reflections there as well.
So, everything seems fine. I cannot test this further right now, since the IOC for the soft motors on elevate somehow doesn't start anymore.

[9:34 PM] Jemian, Pete R.
Right. It replaces a reflection that matches hkl & wavelength (each to 7 digits). Assumes that restore means replace existing reflection with values from the file. Is this the right behavior? (We really should put this discussion in the github PR for others to see.)

[9:46 PM] Strempfer, Joerg
This is actually not what I expected. There might be identical reflections with different angular values, e.g. if the lattice parameter changes with temperature. Right now it restores only the orienting reflections but I think we need to restore all reflections.

In [18]: list_reflections(True)
Sample: main
#   H  K  L    Delta    Theta      Chi      Phi    Gamma       Mu   orienting   
0   0  0  2   40.000   20.000   16.690   10.000    9.230    0.000
1   0  4  0   60.000   30.000    0.000    0.000    0.000    0.000   second       
2   0  0  2   40.000   21.000   16.690   10.000    9.230    0.000   first        
============================================================================
Sample: EuAl4
 
#   H  K  L    Delta    Theta      Chi      Phi    Gamma       Mu   orienting   
0   0  0  2   40.000   21.000   16.690   10.000    9.230    0.000
1   0  4  0   60.000   30.000    0.000    0.000    0.000    0.000   second       
2   0  0  2   40.000   22.000   16.690   10.000    9.230    0.000
3   0  0  2   40.000   22.000   16.690   10.000    9.230    0.000   first        
============================================================================

In [19]: write_config()
Overwrite existing configuration file (y/[n])? y
Writing configuration file.
 
In [20]: read_config()
Read configuration file 'diffract-config.json'.
Method ([o]verwrite/[a]ppend)? o
 
In [21]: list_reflections(True)
Sample: main
 
#   H  K  L    Delta    Theta      Chi      Phi    Gamma       Mu   orienting   
0   0  4  0   60.000   30.000    0.000    0.000    0.000    0.000   first        
1   0  0  2   40.000   21.000   16.690   10.000    9.230    0.000   second       
============================================================================
Sample: EuAl4
 
#   H  K  L    Delta    Theta      Chi      Phi    Gamma       Mu   orienting   
0   0  4  0   60.000   30.000    0.000    0.000    0.000    0.000   first        
1   0  0  2   40.000   22.000   16.690   10.000    9.230    0.000   second       
============================================================================

I just see it restores also other reflections than the orienting ones but only one of a kind.

[prjemian]

Since there is no update_reflection() code available (hint for v2 feature), the restore procedure removes a matching reflection before adding it (as a new reflection). Avoids duplicating a reflection.

hklpy/hkl/configuration.py

Lines 298 to 300 in bf8e0bb

	refl = reflection.find(sample)
	if refl is not None:
	sample.remove_reflection(refl)

[prjemian]

Under what circumstances would the same hkl & wavelength be used with different angles? You said for different sample temperatures? How could you use these to compute a UB matrix? They have different lattice spacings. That changes the UB matrix.

[padraic-shafer]

Re: __init__ --> tools, I'm honored. :)

The tests look good too (via visual inspection...unable to install libhkl on macos).

[padraic-shafer]

Under what circumstances would the same hkl & wavelength be used with different angles? You said for different sample temperatures? How could you use these to compute a UB matrix? They have different lattice spacings. That changes the UB matrix.

In practice, it's very common to not change the stored lattice basis for a sample during an experiment, but instead to encode the lattice expansion/contraction as "changes" in (hkl) values of an observed reflection. Alternatively some beamline conventions keep (hkl) of the reflection fixed, and "absorb" the lattice changes by allowing the diffraction angles for that reflection to vary.

You're right that updating the lattice parameters would be the proper physical property to track, but this is usually a buried configuration in scan software that does not get logged and which might involve "resetting" the system. So tracking changes in (hkl) or diffraction angles is a common coping mechanism to keep an experiment running.

I won't speak for @strempfer here, but wanted to share that this is a fairly common practical approach.

[strempfer]

Reflections are always saved with integer hkl but more or less different angles. So right now only the last added reflection (I hope) is restored. Usually this is sufficient but what about a flag allowing to restore all reflections?

[prjemian]

It's easy to make a change here. Let me be clear on what is to be done. In this code, an identical reflection is found by matching both wavelength and hkl to some numerical precision.

hklpy/hkl/configuration.py

Lines 227 to 232 in bf8e0bb

	matches = [
	equal(rdict["reflection"][axis], self.reflection[axis])
	for axis in self.reflection
	] + [
	equal(rdict["wavelength"], self.wavelength)
	]

I'm trying to avoid the situation where restore(clear=False) increases the number of reflections by appending the same reflections to the existing ones.

Both of you are saying it is common practice to have multiple reflections that satisfy this match. The change would also compare the angles. If wavelength, hkl, and angles all match to some numerical precision, then it is the same reflection, right?

[strempfer]

Correct!

[padraic-shafer]

Both of you are saying it is common practice to have multiple reflections that satisfy this match. The change would also compare the angles. If wavelength, hkl, and angles all match to some numerical precision, then it is the same reflection, right?

Just thinking aloud here...

I think this approach will bump into troubles because "to some numerical precision" will depend very much on the sample and experiment, and retrieving multiple matches might not add much value without additional context.

Another approach might be to allow additional key:value pairs to be stored with the sample/reflection...such as temperature. When restoring, the calling user could specify additional keys (..., temperature=sample.temperature) to use for matching the stored reflection. This might add more complexity than you want to handle in this way, so you know, feel free to shoot this down. :)

[prjemian]

How about a label for reach reflection? The user could put that metadata in the label. It the label could be autogenerated if the user did not provide. A label would make it much easier to prevent the addition of additional copies of the same reflection.

…

On Thu, Nov 9, 2023, 8:32 AM Padraic Shafer ***@***.***> wrote: Both of you are saying it is common practice to have multiple reflections that satisfy this match. The change would also compare the angles. If wavelength, hkl, and angles all match to some numerical precision, then it is the same reflection, right? *Just thinking aloud here...* I think this approach will bump into troubles because "to some numerical precision" will depend very much on the sample and experiment, and retrieving multiple matches might not add much value without additional context. Another approach might be to allow additional key:value pairs to be stored with the sample/reflection...such as temperature. When restoring, the calling user could specify additional keys (..., temperature=sample.temperature) to use for matching the stored reflection. This might add more complexity than you want to handle in this way, so you know, feel free to shoot this down. :) — Reply to this email directly, view it on GitHub <#292 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AARMUMH6XQ7WPFPAT3MBPT3YDTSQVAVCNFSM6AAAAAA7DUWSC2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBTHE2DIMBZGI> . You are receiving this because you were assigned.Message ID: ***@***.***>

[padraic-shafer]

How about a label for reach reflection? The user could put that metadata in the label. It the label could be autogenerated if the user did not provide. A label would make it much easier to prevent the addition of additional copies of the same reflection.

I like that very much.

[strempfer]

Wouldn't it be best to just append all reflections without checking? This is what I would expect for a clear=False. There is the possibility for the user to delete unwanted reflections afterwards.

[strempfer]

A label would still be great.

[prjemian]

This is easy for the programmer, given the way that reflections now are stored in the back end library. Seems messy for the user. We can save the label idea for v2 where things like this are designed in, not added as an afterthought.

…

On Thu, Nov 9, 2023, 8:47 AM Jörg Strempfer ***@***.***> wrote: Wouldn't it be best to just append all reflections without checking? This is what I would expect for a clear=False. There is the possibility for the user to delete unwanted reflections afterwards. — Reply to this email directly, view it on GitHub <#292 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AARMUMDR4JEN24NBM3MXGA3YDTUHPAVCNFSM6AAAAAA7DUWSC2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBTHE3TANZWG4> . You are receiving this because you were assigned.Message ID: ***@***.***>

[padraic-shafer]

1. Append all reflections (with labels) sounds reasonable.
2. Future upgrade of restore only matching labels might be something that becomes desirable if there are too many similar reflections.

[prjemian]

With reflection labels, the config file format would have to change. Reflections would be a dictionary rather than a list. Because of that, I'd love to do this as soon as possible. In this branch.

…

On Thu, Nov 9, 2023, 8:48 AM Jörg Strempfer ***@***.***> wrote: A label would still be great. — Reply to this email directly, view it on GitHub <#292 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AARMUMABO3XYAWMACY2D7FLYDTUNJAVCNFSM6AAAAAA7DUWSC2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBTHE3TGNBVGA> . You are receiving this because you were assigned.Message ID: ***@***.***>

[prjemian]

Reflections are always saved with integer hkl but more or less different angles. So right now only the last added reflection (I hope) is restored. Usually this is sufficient but what about a flag allowing to restore all reflections?

For crystallographers, hkl are Miller indices and are always integers. In practice, some use a reflection as a fiducial marker to note a setting of reciprocal and real-space positions. In such case, thinking diffues scattering as the example, hkl could be non-integer. It's more general to keep as a float.

[prjemian]

Let's implement reflection labels in a branch from 289-restore-reflections.

[prjemian]

There are several use cases for a set of reflections:

• computation of the orientation matrix (for 2 or more reflections)
• documentation of observed (or theoretical) reflection settings
• reference coordinates to re-position the diffractometer (move to this one)
• define a crystallographic zone or axis to guide the diffractometer for measurements

[prjemian]

While reflection labels sound like a way to recognize identical reflections, their addition will touch many modules and could introduce breaking changes. Breaking changes are beyond the intent of this PR. The plans for v2 include such breaking changes.

Summary: Not going to add reflection labels now. Instead, when clear=False, then will blindly append reflections from config to existing set. That is easy and will not represent a departure from what users know how to do now.

[prjemian]

@strempfer: I have pushed the latest changes now. Can you git pull in your branch and test again?

[strempfer]

Looks good. Don't see any problems. Thanks!

[strempfer]

Actually, what did you modify? Does it look at the different angles now? It looks like in both overwrite and append mode, it only reads one reflection of a kind even that angles are different.

[strempfer]

Did a git pull again and now it works as it should.

[mrakitin]

The changes make sense to me and the test coverage is good.