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 R-split (optionally) to reported merging stats #2314

Merged
merged 10 commits into from
Jan 18, 2023
Merged

Conversation

jbeilstenedmands
Copy link
Contributor

The R-split metric is a quantity often used in serial crystallography. It is defined as the R-factor between merged half sets of intensities.
For dials.scale and dials.merge, if additional_stats=True, R-split is added to the reported merging stats "xia2-style" summary, and the resolution dependent values are reported in the dials.merge.log and plotted in the dials.merge.html.

@jbeilstenedmands jbeilstenedmands changed the title Add rsplit (optionally) to reported merging stats Add R-split (optionally) to reported merging stats Jan 12, 2023
@codecov
Copy link

codecov bot commented Jan 13, 2023

Codecov Report

Merging #2314 (f6e3eaa) into main (03972ed) will increase coverage by 0.05%.
The diff coverage is 94.28%.

@@            Coverage Diff             @@
##             main    #2314      +/-   ##
==========================================
+ Coverage   80.59%   80.65%   +0.05%     
==========================================
  Files         587      587              
  Lines       67144    67501     +357     
  Branches     8950     9025      +75     
==========================================
+ Hits        54114    54440     +326     
- Misses      10959    10978      +19     
- Partials     2071     2083      +12     

Copy link
Contributor

@rjgildea rjgildea left a comment

Choose a reason for hiding this comment

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

Thanks for this, it will be a useful addition (and the ability to relatively easily include further statistics). I have just tested the PR running dials.merge addititional_stats=True and have the following observations:

  • I see Rsplit reported in the "Summary of merging statistics" table in the log file ✅
  • I see a plot of Rsplit vs resolution in the html output ✅
  • I was surprised that Rsplit wasn't included in the "Merging statistics by resolution bin" table in the log file - how hard would it be to add it here also?
  • Similarly, it is in neither of the tables of statistics reported in the html file - how hard to include here also?

src/dials/algorithms/merging/reporting.py Outdated Show resolved Hide resolved
src/dials/report/analysis.py Outdated Show resolved Hide resolved
src/dials/command_line/merge.py Show resolved Hide resolved
src/dials/command_line/scale.py Outdated Show resolved Hide resolved
@jbeilstenedmands
Copy link
Contributor Author

Thanks for the feedback. I've tried to add it to the log stats table and yes it's possible, just working on some formatting issues now.

@jbeilstenedmands
Copy link
Contributor Author

@rjgildea The latest commits should address all the points raised in your first review

@rjgildea
Copy link
Contributor

Thanks for adding it to the log/html stats table. I've just tested with the latest state of the branch and it is now appearing in the "Merging statistics by resolution bin" log table and the "Merging statistics / Resolution shells" html table. However it seems to have disappeared from the "Summary of merging statistics" log table (and not in the "Merging statistics / Overall" html table either), is this to be expected?

@jbeilstenedmands
Copy link
Contributor Author

However it seems to have disappeared from the "Summary of merging statistics" log table (and not in the "Merging statistics / Overall" html table either), is this to be expected?

Thanks for catching this, I had updated one dict key to Rsplit(I) but missed the other.

I've now added Rsplit to the merging statistics/overall html table as well.

Copy link
Contributor

@rjgildea rjgildea left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, I've now tested again with both dials.scale and dials.merge and can confirm that Rsplit is reported in both summary and by resolution bins in both the logs and html reports, as well as the plot of Rsplit vs resolution.

I did have one question about the absence of a factor 1/2 in your implementation vs the published equation - either I'm missing something or it's possibly a bug.

newsfragments/2314.feature Outdated Show resolved Hide resolved
src/dials/algorithms/scaling/scaling_library.py Outdated Show resolved Hide resolved
src/dials/algorithms/scaling/scaling_library.py Outdated Show resolved Hide resolved
src/dials/command_line/merge.py Outdated Show resolved Hide resolved
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

3 participants