rs-station / reciprocalspaceship

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

Add function to convert structure factor amplitudes to intensities to `algorithms` submodule #156

Closed dennisbrookner closed 2 years ago

dennisbrookner commented 2 years ago

Now with the desired DataSet-based API.

I ended up deciding to name the new columns I_back and SigI_back by default, and to raise a ValueError if the method would overwrite a column. Happy to be overruled on one or both of those.

codecov-commenter commented 2 years ago

Codecov Report

Merging #156 (a4304b8) into main (6d536fa) will decrease coverage by 0.04%. The diff coverage is 95.00%.

@@            Coverage Diff             @@
##             main     #156      +/-   ##
==========================================
- Coverage   98.45%   98.41%   -0.05%     
==========================================
  Files          43       44       +1     
  Lines        1688     1708      +20     
==========================================
+ Hits         1662     1681      +19     
- Misses         26       27       +1     
Flag Coverage Δ
unittests 98.41% <95.00%> (-0.05%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
reciprocalspaceship/algorithms/intensity.py 94.73% <94.73%> (ø)
reciprocalspaceship/algorithms/__init__.py 100.00% <100.00%> (ø)
reciprocalspaceship/io/mtz.py 100.00% <0.00%> (ø)
reciprocalspaceship/dataset.py 98.03% <0.00%> (ø)
reciprocalspaceship/utils/asu.py 100.00% <0.00%> (ø)
reciprocalspaceship/utils/structurefactors.py 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 6d536fa...a4304b8. Read the comment docs.

JBGreisman commented 2 years ago

One last thing -- please add this function to the API reference of the documentation so that it gets auto-documented: https://github.com/Hekstra-Lab/reciprocalspaceship/blob/main/docs/api/index.rst#algorithms

To do this, you just need to add ~reciprocalspaceship.algorithms.compute_intensity_from_structurefactor to the list of functions.

JBGreisman commented 2 years ago

Thanks for sticking with this -- it's looking good to me. Just two small comments:

  1. It looks like a commented chunk of code was included in the last commit -- please remove that for the final commit.
  2. For thoroughness, it seems worthwhile to confirm that F_key and SigF_key select columns with the correct dtype, and to raise a descriptive ValueError if they don't.
dennisbrookner commented 2 years ago

Checking for input types makes a lot of sense to me, but I'm not actually sure I know how to do that. Could you point me towards an example that exists elsewhere in rs?

JBGreisman commented 2 years ago

Your test includes an example close to what I was thinking:

# confirm types of new columns
assert isinstance(result[o1].dtype, rs.IntensityDtype)
assert isinstance(result[o2].dtype, rs.StandardDeviationDtype)

You can just change this to be something like:

if not isinstance(ds.dtypes[F_key], rs.StructureFactorAmplitudeDtype):
    raise ValueError(...)
if not isinstance(ds.dtypes[SigF_key], rs.StandardDeviationDtype):
    raise ValueError(...)
JBGreisman commented 2 years ago

All of the code looks good to me, but please doublecheck your test of the F_key, SigF_key dtypes -- I don't think the test actually hits the relevant lines as written