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

Handle shared experiment models when converting to cambridge frame in mtz export #2182

Merged
merged 2 commits into from
Aug 2, 2022

Conversation

jbeilstenedmands
Copy link
Contributor

If an experiment list has shared experiment models e.g. shared Beam model, the transformations end up being attempted multiple times as the function loops over experiments. I found that this caused a crash in the rotate beam function, but in theory it could lead to an issue in any shared model. So simply make a deepcopy of the model if the model is shared.

Copy link
Member

@dagewa dagewa left a comment

Choose a reason for hiding this comment

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

Good spot. As MTZ does not handle shared models anyway, making distinct copies first to simplify later bookkeeping makes sense.

@codecov
Copy link

codecov bot commented Aug 1, 2022

Codecov Report

Merging #2182 (07b943c) into main (3543e58) will decrease coverage by 0.09%.
The diff coverage is 14.28%.

@@            Coverage Diff             @@
##             main    #2182      +/-   ##
==========================================
- Coverage   80.66%   80.57%   -0.10%     
==========================================
  Files         588      588              
  Lines       66581    66595      +14     
  Branches     9384     9392       +8     
==========================================
- Hits        53707    53658      -49     
- Misses      10798    10853      +55     
- Partials     2076     2084       +8     

@jbeilstenedmands
Copy link
Contributor Author

I think this might also need backporting to the LTS branch, as it fixes a bug introduced in #2054 which was also backported to the LTS branch

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.

None yet

4 participants