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

Ensure invert_rotation_axis=True is only applied once to each goniometer model #2469

Conversation

dagewa
Copy link
Member

@dagewa dagewa commented Jul 26, 2023

More generally, ensure manual geometry changes are only applied once to each unique model.

@codecov
Copy link

codecov bot commented Jul 26, 2023

Codecov Report

Merging #2469 (34ead2b) into main (bcd31c6) will decrease coverage by 0.02%.
Report is 1 commits behind head on main.
The diff coverage is 89.87%.

❗ Current head 34ead2b differs from pull request most recent head 77516da. Consider uploading reports for the commit 77516da to get more accurate results

@@            Coverage Diff             @@
##             main    #2469      +/-   ##
==========================================
- Coverage   78.61%   78.60%   -0.02%     
==========================================
  Files         607      607              
  Lines       74339    74410      +71     
  Branches    10118    10130      +12     
==========================================
+ Hits        58445    58488      +43     
- Misses      13735    13759      +24     
- Partials     2159     2163       +4     

@dagewa
Copy link
Member Author

dagewa commented Jul 28, 2023

@graeme-winter I realise I'm tagging you a lot for PR reviews lately, but you came up in the suggestions 😉

Copy link
Contributor

@graeme-winter graeme-winter left a comment

Choose a reason for hiding this comment

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

On the balance I am fairly sure that the change set meets the brief of fixing the issue as the added test exercises the right code paths. Some concerns:

  • the motif of if scan and scan not in thing scan = worries me slightly as I am nervous that it does not do what we may intend
  • I am left with the idea that invert_rotation_axis is fundamentally wrong, because the other overrides are idempotent but this one is a toggle (which gives rise to the fundamental issue)

On the latter - I note we do not offer invert_beam or invert_detector_origin etc. - however I also recognise that the inverted rotation axis is by far and away the most common issue. Just left with a slight sense of unease, which maybe I should document in a subsequent issue.

The change set I believe meets the brief and so is a good addition to the code base, just raises questions for me.

newsfragments/2469.bugfix Outdated Show resolved Hide resolved
src/dials/command_line/dials_import.py Show resolved Hide resolved
src/dials/command_line/dials_import.py Show resolved Hide resolved
tests/command_line/test_import.py Show resolved Hide resolved
tests/command_line/test_import.py Show resolved Hide resolved
@dagewa dagewa merged commit 4b86bb6 into main Aug 4, 2023
16 checks passed
@dagewa dagewa deleted the 2467-dialsimport-invert_rotation_axis=true-fails-for-even-numbers-of-imagesets branch August 4, 2023 08:48
toastisme pushed a commit to toastisme/dials that referenced this pull request Aug 21, 2023
…are only applied once to each model (dials#2469)

Use a set to keep track of which models have already been manually updated, to avoid doing this more than once
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dials.import invert_rotation_axis=true fails for even numbers of imagesets
3 participants