-
Notifications
You must be signed in to change notification settings - Fork 46
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
Improve handling of profile/sum intensities for scaling. #1300
Conversation
If only one estimate present, use that if intensity_choice=combine
For reference, testing on the X4-wide dataset with the change from #1297 masked_refl_profile_fitting + master scaling, d_min=1.1
masked_refl_profile_fitting + this PR, d_min=1.1
So an increase from 75598 to 91798 overall reflections, however merging stats look worse, although I don't like to read too much into the quality of the x4 wide dataset. More suitable datasets to follow. |
Okay, so ran this on the I23 FutA dataset from #1291 with #1297 applied also.
Running dials.merge on the scaled files with a comparable d_min=1.93 to compare to #1291:
i.e. for d_min=1.93 we now have 148992 reflections compared to 64696 for xia2-3dii and 26876 for xia2-dials master. However lack of anomalous signal still seems an issue. |
elif all(i in intensity_choice for i in ["sum | profile"]): | ||
reducer = SumORPrfIntensityReducer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be clearer?
elif all(i in intensity_choice for i in ["sum | profile"]): | |
reducer = SumORPrfIntensityReducer | |
elif {"sum | profile"} <= set(intensity_choice): | |
reducer = SumORPrfIntensityReducer |
also does intensity_choice need to be a list or can it be a genuine set given that as far as I can tell we don't care about the order of elements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, this can be a set rather than a list
If using intensity_choice=combine in scaling (the default), also use reflections where only one of profile/sum intensity estimate is available. i.e. combine estimates if both available, else just use the one estimate that succeeded.
Current behaviour is that only reflections with both profile and summation estimates are used.
This bit of the scaling code could do with refactoring a bit, however leaving for now to give a clean changeset for if we want to add this to current releases.