rs-station / reciprocalspaceship

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

Add return_edges argument to DataSet.assign_resolution_bins() #187

Closed JBGreisman closed 1 year ago

JBGreisman commented 1 year ago

Fixes #184 by adding a return_edges argument to DataSet.assign_resolution_bins(). This PR adds that flexibility to the API, but otherwise does not change the default behavior of the function to preserve backwards compatibility.

JBGreisman commented 1 year ago

I made the requested refactors to bin_by_percentile() and the corresponding changes to DataSet.assign_resolution_bins() to preserve the user-facing API

codecov-commenter commented 1 year ago

Codecov Report

Base: 98.37% // Head: 98.35% // Decreases project coverage by -0.02% :warning:

Coverage data is based on head (5af73b1) compared to base (01a3e6c). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #187 +/- ## ========================================== - Coverage 98.37% 98.35% -0.03% ========================================== Files 45 44 -1 Lines 1788 1764 -24 ========================================== - Hits 1759 1735 -24 Misses 29 29 ``` | Flag | Coverage Δ | | |---|---|---| | unittests | `98.35% <100.00%> (-0.03%)` | :arrow_down: | 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=Hekstra-Lab#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/Hekstra-Lab/reciprocalspaceship/pull/187?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Hekstra-Lab) | Coverage Δ | | |---|---|---| | [reciprocalspaceship/dataset.py](https://codecov.io/gh/Hekstra-Lab/reciprocalspaceship/pull/187/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Hekstra-Lab#diff-cmVjaXByb2NhbHNwYWNlc2hpcC9kYXRhc2V0LnB5) | `98.26% <100.00%> (+0.04%)` | :arrow_up: | | [reciprocalspaceship/stats/completeness.py](https://codecov.io/gh/Hekstra-Lab/reciprocalspaceship/pull/187/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Hekstra-Lab#diff-cmVjaXByb2NhbHNwYWNlc2hpcC9zdGF0cy9jb21wbGV0ZW5lc3MucHk=) | `100.00% <100.00%> (ø)` | | | [reciprocalspaceship/utils/binning.py](https://codecov.io/gh/Hekstra-Lab/reciprocalspaceship/pull/187/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Hekstra-Lab#diff-cmVjaXByb2NhbHNwYWNlc2hpcC91dGlscy9iaW5uaW5nLnB5) | `89.65% <100.00%> (-0.97%)` | :arrow_down: | | [reciprocalspaceship/\_\_init\_\_.py](https://codecov.io/gh/Hekstra-Lab/reciprocalspaceship/pull/187/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Hekstra-Lab#diff-cmVjaXByb2NhbHNwYWNlc2hpcC9fX2luaXRfXy5weQ==) | | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Hekstra-Lab). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Hekstra-Lab)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

JBGreisman commented 1 year ago

@kmdalton I think this is good to go now, but let me know if you have any other thoughts

JBGreisman commented 1 year ago

i think refls that fall outside the bin range should probably get dropped by default, but i am open to suggestions.

This is different than the current behavior in rs.utils.assign_with_binedges(), but could be implemented in DataSet.assign_resolution_bins()by pre-dropping any reflections before the call. If I do not do the pre-processing, the behavior will be to raise a ValueError that says "This function assumes 'bin_edges' contain every entry in 'data'".

I hesitate to auto-drop reflections from the DataSet, but I think that could be acceptable as long we add a corresponding remark in the docstring

JBGreisman commented 1 year ago

I updated this PR to allow DataSet.assign_resolution_bins() to support making assignments based on explicit bin edges. I made the following decisions regarding API: