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

Ensure compatibility with pandas CoW #842

Merged
merged 7 commits into from
May 23, 2024

Conversation

glatterf42
Copy link
Member

@glatterf42 glatterf42 commented May 22, 2024

Closes #804.
This PR temporarily enables the pandas Copy-on-Write behaviour that will become standard with pandas 3.0. With this, I ran all tests locally to study the warnings we would get from this: only two files were affected, add_year/__init__ and macro. Then, I fixed all of these warnings as suggested in the migration guide, so we might be good with this repo (and all underlying ixmp functions called from the test suite here) :)

How to review

  • Read the diff and note that the CI checks all pass.
  • Study CI output to see if any pandas Warnings wrt CoW came up that were skipped locally.
  • Fix new Warnings (if any).
  • Remove the TEMPORARY commit to avoid enabling pandas CoW for all users already.

PR checklist

  • Continuous integration checks all ✅
  • [ ] Add or expand tests; coverage checks both ✅
  • [ ] Add, expand, or update documentation.
  • Update release notes.

@glatterf42 glatterf42 added the dependencies Pull requests that update a dependency file label May 22, 2024
@glatterf42 glatterf42 added this to the 3.9 milestone May 22, 2024
@glatterf42 glatterf42 requested a review from khaeru May 22, 2024 06:40
@glatterf42 glatterf42 self-assigned this May 22, 2024
Copy link

codecov bot commented May 22, 2024

Codecov Report

Attention: Patch coverage is 71.59091% with 25 lines in your changes are missing coverage. Please review.

Project coverage is 95.6%. Comparing base (4207f5c) to head (4913c24).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##            main    #842     +/-   ##
=======================================
+ Coverage   95.4%   95.6%   +0.1%     
=======================================
  Files         46      46             
  Lines       4354    4339     -15     
=======================================
- Hits        4156    4149      -7     
+ Misses       198     190      -8     
Files Coverage Δ
message_ix/macro.py 96.7% <100.0%> (+<0.1%) ⬆️
message_ix/tools/add_year/__init__.py 66.8% <70.9%> (+0.6%) ⬆️

Copy link
Member

@khaeru khaeru left a comment

Choose a reason for hiding this comment

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

So long as CI passes, we are good to go.

I made some inline comments that maybe would be irrelevant if #494 will refactor the affected code, so they are just FYI.

else:
df = df.reset_index().copy()
df = (
df.reset_index().loc[df.reset_index()[level].isin(locator)].copy()
Copy link
Member

Choose a reason for hiding this comment

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

#494 was intended to pick up and improve many of these usages. Here, I suspect .xs() could be used. No need to actually make that change in this PR, just noting for later reference.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the note :)

message_ix/tools/add_year/__init__.py Outdated Show resolved Hide resolved
message_ix/tools/add_year/__init__.py Outdated Show resolved Hide resolved
@glatterf42
Copy link
Member Author

Not sure why codecov/patch registers a diff hit, we test the same functions as before. So I'm fine with the hit if that comes from additional lines that reduce complexity. I'll rebase to exclude the TEMPORARY commit and merge once the tests pass (again).

@glatterf42 glatterf42 merged commit 97e0cbd into iiasa:main May 23, 2024
23 of 24 checks passed
@glatterf42 glatterf42 deleted the test/pandas-cow branch May 23, 2024 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Comply with pandas 3.0 CoW default
2 participants