Updated functionality for add_rfree and copy_rfree

[dennisbrookner]

Making changes to rs.utils.add_rfree as per #169 and changes to rs.utils.copy_rfree as per #159

add_rfree takes seed

The seed is passed through to np.random.default_rng(seed), such that running add_rfree again with the same seed results in the same flags. seed defaults to None, and conveniently np.random.default_rng(None) is a valid call.

A test was added to test_add_rfree() to confirm that flags are the same when a seed is used, and are different when a seed is not used.

copy_rfree handles column names flexibly

The recent addition of the ccp4_convention parameter to add_rfree means that there are two possible names for an r-free column, "R-free-flags" (phenix convention) or "FreeR_flag" (ccp4 convention). copy_rfree will now by default search the provided dataset_with_rfree for a column named "R-free-flags", then "FreeR_flag", and throw a ValueError if neither is found.

The user can override this behavior via the custom_rfree_key argument if the r-free flag column uses any other naming convention.

A function test_copy_rfree() was added to test successfully copying in all combinations of column naming. A function test_copy_rfree_errors() was added to verify that ValueErrors are thrown when appropriate.

Old unittest code

There is an existing test for copy_rfree written with unittest. I believe that all key functionalities are tested in my new pytest code, making the unittest code redundant, but I left it in for now and labelled it in its docstring that it is a "legacy" version. Let me know if there's anything in the unittest code that I should additionally migrate, and/or if the unittest code should me removed altogether.

[codecov-commenter]

Codecov Report

Merging #170 (6315231) into main (2d279de) will decrease coverage by 0.10%.
The diff coverage is 86.66%.

@@            Coverage Diff             @@
##             main     #170      +/-   ##
==========================================
- Coverage   98.41%   98.30%   -0.11%     
==========================================
  Files          44       44              
  Lines        1704     1713       +9     
==========================================
+ Hits         1677     1684       +7     
- Misses         27       29       +2     

Flag	Coverage Δ
unittests	98.30% <86.66%> (-0.11%)	⬇️

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

Impacted Files	Coverage Δ
reciprocalspaceship/utils/rfree.py	93.75% <86.66%> (-6.25%)	⬇️

---

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 facb76c...6315231. Read the comment docs.

[JBGreisman]

Thanks for taking care of these -- your changes look good to me. My only updates:

1. Please change custom_rfree_key --> rfree_key. I would rather keep the function arguments as short as possible, and I don't think the "custom" adds clarification.
2. Please delete the now obsolete unittest code -- your pytest versions look good to me.

[JBGreisman]

I didn't catch this before, but this can be simplified to:

@pytest.mark.parametrize("rfree_key", [None, "missing key"])
def test_copy_rfree_errors(data_fmodel, rfree_key):
    """
    Test expected ValueErrors for rs.utils.copy_rfree()
    """
    with pytest.raises(ValueError):
        rs.utils.copy_rfree(data_fmodel, data_fmodel, rfree_key=rfree_key)

I think the above is true... let me know if I'm mistaken

[dennisbrookner]

Oo good call, I think you're right. Didn't occur to me because it's a different ValueError, but yeah this should work.

[dennisbrookner]

I'll just clarify in the docstring what the two ValueErrors are, because I think the condensed version is a little harder to parse

[dennisbrookner]

In theory a slightly more rigorous test would be to confirm that the "missing key" case throws an error even when "R-free-flag" or "FreeR_flags" is present, but I think that's probably overkill.

[JBGreisman]

yep, I think your comment there is good.

I think the test you just mentioned is a pretty good one because it tests the intended implementation (that rfree_key supercedes any other default column names). If we change the order of the if clauses in the future, that could change, so I think those sorts of tests are useful for making sure as many corner cases are covered.

[JBGreisman]

Looks good -- thanks for making the fixes!