Add `sample_rate` and `dmin` arguments to `DataSet.to_reciprocalgrid()`

[JBGreisman]

This PR addresses #175 by updating the DataSet.to_reciprocalgrid() call signature to accept sample_rate and dmin arguments. The default value for sample_rate=3.0 and the default for dmin=None, which corresponds to using the full resolution of the DataSet instance. The to_reciprocalgrid() method can still accept an explicit gridsize, and if one is provided, it supersedes the sample_rate and dmin arguments.

To facilitate this method, I also added rs.utils.get_gridsize(), which can be used for calculating a proper gridsize for a DataSet instance.

Lastly, this PR cleans up and adds additional tests to test_dataset_grid.py to more thoroughly test this function.

[codecov-commenter]

Codecov Report

Merging #179 (613d35c) into main (99883ed) will increase coverage by 0.03%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #179      +/-   ##
==========================================
+ Coverage   98.32%   98.36%   +0.03%     
==========================================
  Files          44       45       +1     
  Lines        1731     1772      +41     
==========================================
+ Hits         1702     1743      +41     
  Misses         29       29              

Flag	Coverage Δ
unittests	98.36% <100.00%> (+0.03%)	:arrow_up:

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

Impacted Files	Coverage Δ
reciprocalspaceship/dataset.py	98.15% <100.00%> (+0.11%)	:arrow_up:
reciprocalspaceship/utils/__init__.py	100.00% <100.00%> (ø)
reciprocalspaceship/utils/grid.py	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[JBGreisman]

One API decision here that I'd like feedback on is whether rs.utils.get_gridsize() is the right place for that function. Alternatively, should that be a method of the DataSet class?

[kmdalton]

I think a get_fourier_gridsize type of function should exist in utils because it seems generally useful especially in the context of initializing deep learning models. I don't think this get_gridsize function belongs in utils though. In my mind nothing in utils should require a dataset instance. Let's do this at the top level for now. If you want a separate method to handle the grid sizing, I'd suggest writing a "private" DataSet._get_gridsize helper.

I can do a more thorough review tomorrow.

[JBGreisman]

Thanks for the comment -- I agree that as written the function wasn't right for rs.utils, which we try to keep agnostic to any implementation/interface details in DataSet.

I just pushed an update to this PR that changes the rs.utils.get_gridsize()call signature to be more general. I also added a specific helper function DataSet.get_gridsize(), which is then called in DataSet.to_reciprocalgrid().

[JBGreisman]

I made the requested call signature and method name changes -- I agree that they all clarify things, and make things more consistent with our naming scheme.

The big additional API change is that I changed DataSet.to_reciprocalgrid()--> DataSet.to_reciprocal_grid() to make things as consistent as possible. Since this change will break backwards compatibility, I have kept DataSet.to_reciprocalgrid(key, gridsize=...) and added a DeprecationWarning. I have also updated the documentation with a warning, and have changed the example notebooks to use the new function. In a future release we can fully remove support for the deprecated version.

[JBGreisman]

Made the two changes -- thanks for the review! Please feel free to merge if you think this is good to go

[kmdalton]

Sorry -- I forgot to point out that the call to get_reciprocal_grid_size still had a utils prefix. I just pushed a commit to fix. The CI should pass now :crossed_fingers:

[JBGreisman]

Thanks -- good catch.