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

Update rs.DataSet.reset_index() call signature to matches pandas >v1.5 #266

Merged
merged 3 commits into from
Aug 16, 2024

Conversation

JBGreisman
Copy link
Member

There was an update to the pandas API in v1.5 that changed the pd.DataFrame.reset_index() call signature. This PR updates the call signature of rs.DataSet.reset_index() and adds tests to confirm the consistency of the call signature between:

  • rs.DataSet.reset_index() and pd.DataFrame.reset_index()
  • rs.DataSet.set_index() and pd.DataFrame.set_index()

@JBGreisman JBGreisman added bug Something isn't working API Interface Decisions version Version issues labels Aug 16, 2024
@JBGreisman JBGreisman requested a review from kmdalton August 16, 2024 16:22
Copy link
Member

@kmdalton kmdalton left a comment

Choose a reason for hiding this comment

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

I'm going to approve this, because it seems sensible and fixes the bug, but I wonder if it'd be better to use a more robust approach based on functools.wraps and *args, **kwargs.

tests/test_dataset_signatures.py Show resolved Hide resolved
reciprocalspaceship/dataset.py Show resolved Hide resolved
tests/test_dataset_signatures.py Show resolved Hide resolved
@JBGreisman
Copy link
Member Author

I'm open to the idea of the *args/**kwargs paradigm and we do use it elsewhere. Personally, I find these to be very frequently used methods, so I enjoy having the easy introspection of call signature and correct docstrings (that don't mention pandas specifics). That was the main rationale for keeping it this way. Definitely open to reconsidering if you feel otherwise though.

@kmdalton
Copy link
Member

I'm open to the idea of the *args/**kwargs paradigm and we do use it elsewhere. Personally, I find these to be very frequently used methods, so I enjoy having the easy introspection of call signature and correct docstrings (that don't mention pandas specifics). That was the main rationale for keeping it this way. Definitely open to reconsidering if you feel otherwise though.

I think there are good reasons not to use *args/ **kwargs in this case. It's quite an important method. I was just trying to safeguard against changes in Pandas breaking the CI. I think maybe the CI should break in this case. We should hear about it whenever their call signature changes. I'm good with this.

@JBGreisman JBGreisman merged commit 1979862 into main Aug 16, 2024
8 checks passed
@JBGreisman JBGreisman deleted the pandas_signatures branch August 16, 2024 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Interface Decisions bug Something isn't working version Version issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants