search

rs-station / reciprocalspaceship

Tools for exploring reciprocal space
https://rs-station.github.io/reciprocalspaceship/
MIT License
28 stars 12 forks source link

clip I for Sigma calc #235

Closed DHekstra closed 9 months ago

DHekstra commented 9 months ago

When using scale_merged_intensities to perform French-Wilson scaling in the anisotropic mode, the local Sigma is calculated by a local weighted average of observed intensities. As a consequence, Sigma can be negative. This leads to NaNs in the calculation of the logpdf used to calculate the posterior expected intensity. To remedy this, I propose clipping the observed intensities to be positive in (and only in) the calculation of Sigma in mean_intensity_by_miller_index(I, H, bandwidth).

codecov-commenter commented 9 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (0e2f5e2) 92.39% compared to head (d25d7da) 92.40%. Report is 3 commits behind head on main.

:exclamation: Current head d25d7da differs from pull request most recent head f7f472e. Consider uploading reports for the commit f7f472e to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #235 +/- ## ======================================= Coverage 92.39% 92.40% ======================================= Files 37 37 Lines 2434 2435 +1 ======================================= + Hits 2249 2250 +1 Misses 185 185 ``` | [Flag](https://app.codecov.io/gh/rs-station/reciprocalspaceship/pull/235/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=rs-station) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/rs-station/reciprocalspaceship/pull/235/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=rs-station) | `92.40% <100.00%> (+<0.01%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=rs-station#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

kmdalton commented 9 months ago

I'm not certain that fw should really be applied to data with resolution shells that have negative average intensity. Silently clipping the values is not a good solution if it leads to the user unknowingly processing such data. If you want to merge a change like this, it needs to print a warning or better yet be implemented as an optional flag which is disabled by default.

DHekstra commented 9 months ago

That makes sense. In my single test, Sigma was negative for ~0.01% of reflections. I'll think some more about this when time permits.