Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

standardize export & restore of diffractometer configuration #279

Merged
merged 98 commits into from
Nov 2, 2023

Conversation

prjemian
Copy link
Contributor

@prjemian prjemian added this to the v1.1 milestone Oct 26, 2023
@prjemian prjemian self-assigned this Oct 26, 2023
@prjemian prjemian marked this pull request as ready for review October 26, 2023 22:11
@prjemian
Copy link
Contributor Author

prjemian commented Oct 26, 2023

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

Copy link
Contributor

@padraic-shafer padraic-shafer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO (for me, tomorrow):

  • Review 'hkl/configuration.py'
  • Review 'tests/conftest.py'
  • Review 'tests/test_configuration.py'

hkl/util.py Outdated Show resolved Hide resolved
hkl/tests/test_tardis.py Outdated Show resolved Hide resolved
hkl/tests/test_tardis.py Outdated Show resolved Hide resolved
hkl/tests/test_tardis.py Outdated Show resolved Hide resolved
Copy link
Contributor

@padraic-shafer padraic-shafer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really useful feature, and nicely done. I left some comments and suggestions, but none of them are fatal.

hkl/tests/test_configuration.py Outdated Show resolved Hide resolved
hkl/tests/test_configuration.py Outdated Show resolved Hide resolved
hkl/tests/test_configuration.py Outdated Show resolved Hide resolved
hkl/configuration.py Outdated Show resolved Hide resolved
hkl/configuration.py Outdated Show resolved Hide resolved
hkl/configuration.py Outdated Show resolved Hide resolved
hkl/configuration.py Outdated Show resolved Hide resolved
hkl/configuration.py Outdated Show resolved Hide resolved
hkl/configuration.py Outdated Show resolved Hide resolved
hkl/tests/test_configuration.py Outdated Show resolved Hide resolved
@prjemian
Copy link
Contributor Author

prjemian commented Nov 1, 2023

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
Copy link
Contributor Author

prjemian commented Nov 1, 2023

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

Copy link
Member

@mrakitin mrakitin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very nice addition and great test coverage!
I added a few non-blocking comments.

docs/source/configuration.rst Show resolved Hide resolved
Comment on lines -410 to +451
return dict(
reflection=dict(h=h, k=k, l=l),
flag=refl.flag_get(),
wavelength=geom.wavelength_get(1),
position={k: v for k, v in zip(geom.axis_names_get(), geom.axis_values_get(1))},
orientation_reflection=refl in self._orientation_reflections,
)
return {
"reflection": {"h": h, "k": k, "l": l},
"flag": refl.flag_get(), # not used by hklpy
"wavelength": geom.wavelength_get(1),
"position": dict(zip(geom.axis_names_get(), geom.axis_values_get(1))),
"orientation_reflection": refl in self._orientation_reflections,
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did not dict work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dict(zip(geom.axis_names_get(), geom.axis_values_get(1))) was still used, not sure if there is a huge benefit, but it's OK.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. That was the form suggested, rather than a dict comprehension. Go figure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pylint flagged the simpler dict(a=1, b="two") style for replacement with {"a": 1, "b": "two"}. Looks cumbersome to me and adds invisible benefit for such small dictionaries. But it makes pylint happy.

docs/source/configuration.rst Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we need to add this file and other non-Python files to https://github.com/bluesky/hklpy/blob/main/MANIFEST.in.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, why not to reuse docs/source/_static/e4c-config.json?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2146ed2. Unit test is in-source.

hkl/util.py Show resolved Hide resolved

before.pop("datetime")
after.pop("datetime")
assert before == after, "should be same configuration"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We found it useful to use https://pypi.org/project/dictdiffer/ to compare large/nested dictionaries. It may be useful here as well.

hkl/configuration.py Outdated Show resolved Hide resolved
hkl/configuration.py Outdated Show resolved Hide resolved
"""

def __init__(self, diffractometer):
from .diffract import Diffractometer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not import it on the top of the file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid potential for circular import.

hkl/configuration.py Outdated Show resolved Hide resolved
Copy link
Member

@mrakitin mrakitin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@prjemian prjemian merged commit e7abba8 into main Nov 2, 2023
8 checks passed
@prjemian prjemian deleted the 256-orientation-dict branch November 2, 2023 14:02
@strempfer
Copy link
Collaborator

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
Copy link
Contributor Author

prjemian commented Nov 7, 2023

Sounds like a bug. Thanks for the report!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Store operational information locally for use outside of bluesky
5 participants