bluesky / hklpy

Diffractometer computation library with ophyd pseudopositioner support
https://blueskyproject.io/hklpy
BSD 3-Clause "New" or "Revised" License
2 stars 11 forks source link

standardize export & restore of diffractometer configuration #279

Closed prjemian closed 8 months ago

prjemian commented 9 months ago
prjemian commented 9 months ago

@mrakitin @klauer This is ready for review. It's a major part of the coming v1.1 release.

prjemian commented 9 months ago

@padraic-shafer Thanks for the review. I'll attend to those items you mentioned and also wait for Max's review next week. Is there anyone else who we should ask to comment? Perhaps: @strempfer, @ambarb, @rodolakis, @gfabbris

prjemian commented 9 months ago

Ok, wait on further review of this PR, pending a total revision of the configuration.py (and consequently tests and package requirements) based on this suggestion by @klauer.

In a side branch from this one.

prjemian commented 9 months ago

Switched to use dataclasses and apischema for validation, all tests pass, marking ready for review again.

prjemian commented 9 months ago

With that last change, it may be necessary to add

.. autoclass:: MyClass
   :members:

   .. automethod:: __init__

if an __init__() method is to be documented.

prjemian commented 9 months ago

Now, the util.new_lattice() function appears in the Sphinx docs.

prjemian commented 9 months ago

All current conversations are resolved (commit hashes when appropriate show the changed code). I'd like to merge this PR by the end of the week. Any other items to resolve?

prjemian commented 8 months ago

The diffractometer name and wavelength should be optional. They are not needed to restore a configuration.

prjemian commented 8 months ago

In discussion, @rodolakis is concerned that the constraints should not be restored. At least, it should be optional whether to restore the constraints since there may be hardware limitations for improper settings, or users may fail to achieve a solution for given $(hkl)$.

prjemian commented 8 months ago

While a nice thing to have, the $U$ matrix is not an absolute requirement. This could be moved to optional.

strempfer commented 8 months ago
In [61]: config.restore(pathlib.Path("fourc-config.json"), clear=True)

In [62]: list_reflections(True)
Sample: main

 #   H  K  L   Two Theta    Theta      Chi      Phi   orienting   
 0   2  0  0      37.000    0.000   90.000   71.500   first        
 1   0  4  0      60.000   30.000   90.000    0.000   second       
============================================================================
Sample: EuAl4

 #   H  K  L   Two Theta    Theta      Chi      Phi   orienting   
 0   0  4  0      60.000   30.000   90.000    0.000   first        
 1   2  0  0      37.000    0.000   90.000   71.500   second       
============================================================================

In [63]: config.restore(pathlib.Path("fourc-config.json"), clear=False)

In [64]: list_reflections(True)
Sample: main

 #   H  K  L   Two Theta    Theta      Chi      Phi   orienting   
 0   2  0  0      37.000    0.000   90.000   71.500   first        
 1   0  4  0      60.000   30.000   90.000    0.000   second       
============================================================================
Sample: EuAl4

 #   H  K  L   Two Theta    Theta      Chi      Phi   orienting   
 0   0  4  0      60.000   30.000   90.000    0.000 
 1   2  0  0      37.000    0.000   90.000   71.500 
 2   2  0  0      37.000    0.000   90.000   71.500 
 3   0  4  0      60.000   30.000   90.000    0.000 
 4   0  4  0      60.000   30.000   90.000    0.000   first        
 5   2  0  0      37.000    0.000   90.000   71.500   second       
============================================================================

It looks like when clear=False, it reads reflections from 'main' also into the following sample.

prjemian commented 8 months ago

Sounds like a bug. Thanks for the report!