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

Add a 'scaled' flag to the reflection table. #1377

Merged
merged 6 commits into from
Sep 10, 2020
Merged

Conversation

jbeilstenedmands
Copy link
Contributor

Introduce a 'scaled' flag, which is set at the end of scaling to indicate which are the good scaled reflections (not excluded, not outliers etc). This is the inverse of the current 'bad_for_scaling' flag, which is unclear and prone to misuse, as indicated by the confusion in the discussion in #1268.

Ideally, the scaled flag will replace all uses of 'bad_for_scaling' outside scaling code - this has not been implemented yet, as it will require updating some of the regression test files (e.g. l_cysteine_4_sweeps_scaled). If we're happy with the proposed changes here, then once merged I can update the test files and implement further changes of bad_for_scaling to scaled in places such as dials.util.filter_reflections

@codecov
Copy link

codecov bot commented Aug 14, 2020

Codecov Report

Merging #1377 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1377      +/-   ##
==========================================
+ Coverage   64.70%   64.71%   +0.01%     
==========================================
  Files         616      616              
  Lines       69735    69762      +27     
  Branches     9546     9549       +3     
==========================================
+ Hits        45120    45145      +25     
- Misses      22835    22837       +2     
  Partials     1780     1780              

@jbeilstenedmands jbeilstenedmands marked this pull request as ready for review August 25, 2020 10:15
@ndevenish
Copy link
Member

I think we're ready to merge this when the conflicts are resolved

@ndevenish
Copy link
Member

What's the relation between scaled and bad_for_scaling? It seems to set scaled as the exact opposite quite often, but I can't tell if there are scenarios where they diverge.

@ndevenish
Copy link
Member

Also, needs a newsfragment (probably .api?)

@jbeilstenedmands
Copy link
Contributor Author

@ndevenish you are correct, as in the PR description, I'm moving over to the scaled flag in general outside of internal scaling code, although keeping bad_for_scaling for convenience within scaling (at least for now). Newsfragment incoming.

@ndevenish
Copy link
Member

D'oh, that's what I get for reading the code and not the description.

Is the long term idea to properly deprecate the bad_for_scaling?

@jbeilstenedmands jbeilstenedmands merged commit 2478b1e into master Sep 10, 2020
@Anthchirp Anthchirp deleted the scaled_flag branch October 27, 2020 17:57
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.

None yet

2 participants