iiasa / aneris

Harmonization of Emissions Trajectories for Integrated Assessment Models
Apache License 2.0
4 stars 11 forks source link

Address recent pandas deprecation and future warnings #43

Closed coroa closed 2 years ago

coroa commented 2 years ago

Several pandas deprecations are addressed:

Also replaces the use of the ancient xlrd==1.2.0 and xlsxwriter with xlrd 2 and openpyxl. Not sure, whether this could have negative consequences.

coroa commented 2 years ago

I have relaunched CI to confirm whether we still see a core dump. I will also try to look into the regression test, as it is important to pass (confirms all data generated in the 2019 paper is reproduced).

I'll rebase on the new PR #44 , as soon as it seems to solve fine.

coroa commented 2 years ago

I will also try to look into the regression test, as it is important to pass (confirms all data generated in the 2019 paper is reproduced).

In https://github.com/iiasa/aneris/runs/5318435316?check_suite_focus=true#step:8:132 it looks as if in the failing regression test overrides is a Series (so that o["method"] tries to find "method" in the index) rather than a dataframe as i had imagined.

I actually don't know where to find the data to reproduce the regression tests. So yes, i guess you will have to investigate.

gidden commented 2 years ago

I think the easiest way to do this is to simply print out what it was before and is now (so execute the old o code and print the result). I know it's low-brow, but I don't have time to dig into the deep guts here unfortunately.

coroa commented 2 years ago

Congruent with my prior analysis, overrides is actually converted to a Series in the TrajectoryPreprocessor (in the old implementation it did not matter, since reset_index turned it back into a dataframe): https://github.com/iiasa/aneris/blob/491db127e101afe06cd190dd5f0d4f036438557c/aneris/harmonize.py#L302

gidden commented 2 years ago

Good catch @coroa - I can't tell the precise change here now that has caused regression tests to complete, but I presume give full passage this is not good to merge.

Do you prefer that we squash and merge this and close #44 ? Otherwise I can merge #44 now and this would need a rebase or cherry-pick. Either way fine with me.

coroa commented 2 years ago

I can't tell the precise change here now that has caused regression tests to complete, but I presume give full passage this is not good to merge.

Thanks, but that is easy to remedy (and caused by me force-pushing, in the quest for perfect history), instead of just new commits: https://github.com/iiasa/aneris/compare/daf54e01251fdd3bdcb2b6a7c6091eb2f11373d3..65a592e6f3d3d0c4cd65799ba1ffdc8e40c0d7cd#diff-c64e8135852ba8ee259c13be437411b65d316b93790af1c0f9f421b489207829R522 ie. in _get_global_overrides, the o["method"] was only needed after the reset_index call in the legacy implementation, return o is what the simpler implementation requires.

Do you prefer that we squash and merge this and close #44 ? Otherwise I can merge #44 now and this would need a rebase or cherry-pick. Either way fine with me.

Let's squash-merge #44 and then squash merge this one!

gidden commented 2 years ago

44 merged!