Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add hkls attribute rs.DataSet #286

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

add hkls attribute rs.DataSet #286

wants to merge 4 commits into from

Conversation

kmdalton
Copy link
Member

@kmdalton kmdalton commented Dec 6, 2024

This PR makes a .hkls attribute for rs.DataSet instances. The method has a setter to assign new Miller indices. It retains backward compatibility with the get_hkls method.

The use case here is for being able to set new miller indices using an expression like

ds.hkls = hkls

where hkls can either be a dataset containing the keys 'H', 'K', and 'L' or it could be a numpy array. This saves the user having to reset and set the index.

@kmdalton kmdalton requested a review from JBGreisman December 6, 2024 21:39
@codecov-commenter
Copy link

codecov-commenter commented Dec 6, 2024

Codecov Report

Attention: Patch coverage is 70.83333% with 7 lines in your changes missing coverage. Please review.

Project coverage is 88.78%. Comparing base (01286d8) to head (a4fb023).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
reciprocalspaceship/dataset.py 70.83% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #286      +/-   ##
==========================================
- Coverage   88.98%   88.78%   -0.20%     
==========================================
  Files          40       40              
  Lines        2841     2854      +13     
==========================================
+ Hits         2528     2534       +6     
- Misses        313      320       +7     
Flag Coverage Δ
unittests 88.78% <70.83%> (-0.20%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JBGreisman
Copy link
Member

I think the concept of this makes a lot of sense. I think having DataSet.hkls as an attribute is an intuitive accessor for this sort of info, and I agree with maintaining the get_hkls() method for compatibility. I still have to do a bit of a dive into this to see if there are any gotchas/corner cases that seem problematic.

Some questions that will be easy to answer but I'm just writing here as part of my stream of consciousness:

  • Does this work correctly for both range-indexed and HKL-indexed DataSets (yes! this is explicitly tested)
  • What about if there are extra columns in the index?
  • What happens if the provided HKLs do not have the same number of rows as the DataSet? Do they get NaN-padded?

From what I can tell, this all seems fine, though

Copy link
Member

@JBGreisman JBGreisman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few small comments -- overall, looks good

reciprocalspaceship/dataset.py Outdated Show resolved Hide resolved
return hkl

def get_hkls(self):
"""For backwards compatibility retain the get_hkls method in addition to the dataset.hkls attribute"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this docstring should be more of a method description, rather than a rationale for keeping the method

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I changed it. Let me know if you are okay with the update.

if range_index:
ds = ds.reset_index()

hmax = 20
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have two pytest fixtures (hkls and dataset_hkl) that could probably support this test, rather than rolling your own. If the other two are insufficient, maybe we should modify them so that this can be used elsewhere as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think these fixtures are appropriate, because the hkls in this case should be matched to the length of the dataset. it might make sense to use them to test the case where the dataset length and hkl length differ.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we could match these hkls to those in data_merged and/or data_unmerged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants