Update dependencies (Jarmo-fix)

jkikstra commented 9 months ago

Continuation of https://github.com/iiasa/climate-assessment/pull/47, which itself was a continuation of https://github.com/iiasa/climate-assessment/pull/46. It's already (re)based on the current #49. (speaking as 15:50, 30.11.2023)

The idea is that only merging this current branch should do the trick. The other PRs can then be closed.

The PR does the following:

update dependencies, most notably pyam, where pyam.IamDataFrame.require_variable() has been replaced by pyam.IamDataFrame.require_data(), noting that it is not a one-on-one replacement. Additionally, there seems to have been some changes in functionality around empty filtered outputs.
following this, rewrite reclassify_waste_and_other_co2_ar6(), and delete require_var_allyears which was previously only used in reclassify_waste_and_other_co2_ar6()
also delete drop_broken_stuff which does not seem necessary anymore (would be great if @znicholls can check if this is OK)
building on https://github.com/iiasa/climate-assessment/pull/49, there's no issues anymore
add a test test_add_completeness_category, which ensures that add_completeness_category() runs
rewrite add_completeness_category(). NOTE: this causes a minor change in the functionality, where we don't count the variables anymore (this functionality wasn't really used anyway, and more of a legacy of explorations during an earlier projects: so I deleted the auxiliary function count_variables_very_high() which is not necessary anymore. I am not aware of anyone using this "completeness_category" functionality, and we didn't guarantee its continuation
update some test-data expected files, because a new error occurred regarding the "exclude" column in the output meta file (which may have had to do with updated pyam behaviour, too?)
[x] Tests added
[x] Documentation added
[ ] Example added (in the documentation, to an existing notebook, or in a new notebook)
[x] Description in CHANGELOG.rst added (single line such as: (`#XX <https://github.com/iiasa/climate-assessment/pull/XX>`_) Added feature which does something)

jkikstra commented 9 months ago

Woops - still got a remaining local test failing - will dive back in - so hold your horses for the review for a second

jkikstra commented 9 months ago

I'm hopeful all tests should pass this time: https://github.com/iiasa/climate-assessment/actions/runs/7049792409

jkikstra commented 9 months ago

Okay still not working. @phackstock or @znicholls if you see the chance, would one of you be able to check the issue with the remaining 'exclude' column?

Please feel free to just use this branch and push to it (if you coordinate amongst each other to not do double work ;) ).

znicholls commented 9 months ago

if you see the chance, would one of you be able to check the issue with the remaining 'exclude' column?

I think that is what drop_broken_stuff was for. You'll probably need to add that function back in, drop a break point in there to see the issue and then the fix should be relatively straight-forward. I think the issue was that somehow the index is getting doubled by aneris in a way it didn't used to.

znicholls commented 9 months ago

I fixed a few more tests (I think). The last ones that are failing are now more of a mystery to me. It looks like something has changed in post-processing or what gets put in as emissions maybe (at least that's how it seems given there's now only 4 failures and they're all post-processing/climate related)?

jkikstra commented 9 months ago

Thanks for this Zeb. I won't have time today to look at it, but just wanted to quickly jot down things I saw in the latest test outputs and my thoughts:

Potentially my previous "update-expected-files" (https://github.com/iiasa/climate-assessment/pull/50/commits/16a14916092996478d10ea045f9dd8cda9b96c4f) may have caused some issues now that you added back in your hack, we can try reverting it?

Following a deprecation warning I now notice, we should get rid of the pyam.IamDataFrame.reset_exclude() function:

/home/runner/work/climate-assessment/climate-assessment/src/climate_assessment/checks.py:880: DeprecationWarning: This method is deprecated and will be removed in future versions. Use `IamDataFrame.exclude = False` instead.
    df.reset_exclude()

The following suggests to me that there's still a mismatch between expected and produced 'exclude' column outputs (which I didn't understand last time, because it didn't seem to be in the real outputs):

meta ex2_harmonized_infilled_alloutput.xlsx shape mismatch
[left]:  (48, 12)
[right]: (49, 12)

On my laptop, I now actually get different fails/errors (KeyError) messages than online, not sure whether something is up with my installation leading to different tests/files used when I do pytest tests:

=============================================== short test summary info ===============================================
FAILED tests/integration/test_harmonization_and_infilling.py::test_harmonize_and_infill_negative_co2_afolu_in_input - KeyError: 'variable'
FAILED tests/integration/test_infillerdatabase.py::test_example_small_infillerdatabase - AssertionError: None
FAILED tests/integration/test_workflow_integration.py::test_workflow_historical_warming[0.85-1850-1900-2015-2015] - AssertionError: None
FAILED tests/integration/test_workflow_integration.py::test_workflow_historical_warming[0.92-1900-1920-2020-2020] - AssertionError: None
FAILED tests/regression/test_harmonize.py::test_workflow_harmonization - AssertionError: None
FAILED tests/regression/test_harmonize.py::test_workflow_harmonization_noinputchecks - AssertionError: None
FAILED tests/regression/test_harmonize_and_infill.py::test_workflow_harmonization_and_infilling - AssertionError: None
FAILED tests/regression/test_workflow.py::test_workflow_ciceroscm - AssertionError: KeyError('variable')
FAILED tests/regression/test_workflow.py::test_workflow_fair - AssertionError: None
ERROR tests/integration/test_harmonization.py::test_harmonization_ar6[Emissions|BC] - KeyError: 'variable'
ERROR tests/integration/test_harmonization.py::test_harmonization_ar6[Emissions|CH4] - KeyError: 'variable'
ERROR tests/integration/test_harmonization.py::test_harmonization_ar6[Emissions|CO] - KeyError: 'variable'
ERROR tests/integration/test_harmonization.py::test_harmonization_ar6[Emissions|CO2] - KeyError: 'variable'
ERROR tests/integration/test_harmonization.py::test_harmonization_ar6[Emissions|CO2|AFOLU] - KeyError: 'variable'
ERROR tests/integration/test_harmonization.py::test_harmonization_ar6[Emissions|CO2|Energy and Industrial Processes] - KeyError: 'variable'
ERROR tests/integration/test_harmonization.py::test_harmonization_ar6[Emissions|N2O] - KeyError: 'variable'
ERROR tests/integration/test_harmonization.py::test_harmonization_ar6[Emissions|NH3] - KeyError: 'variable'
ERROR tests/integration/test_harmonization.py::test_harmonization_ar6[Emissions|OC] - KeyError: 'variable'
ERROR tests/integration/test_harmonization.py::test_harmonization_ar6[Emissions|SF6] - KeyError: 'variable'
ERROR tests/integration/test_harmonization.py::test_harmonization_ar6[Emissions|Sulfur] - KeyError: 'variable'
ERROR tests/integration/test_harmonization.py::test_harmonization_ar6[Emissions|VOC] - KeyError: 'variable'
ERROR tests/integration/test_harmonization.py::test_harmonization_sr15[Emissions|BC] - KeyError: 'variable'
ERROR tests/integration/test_harmonization.py::test_harmonization_sr15[Emissions|CH4] - KeyError: 'variable'
ERROR tests/integration/test_harmonization.py::test_harmonization_sr15[Emissions|CO] - KeyError: 'variable'
ERROR tests/integration/test_harmonization.py::test_harmonization_sr15[Emissions|CO2|AFOLU] - KeyError: 'variable'
ERROR tests/integration/test_harmonization.py::test_harmonization_sr15[Emissions|CO2|Energy and Industrial Processes] - KeyError: 'variable'
ERROR tests/integration/test_harmonization.py::test_harmonization_sr15[Emissions|N2O] - KeyError: 'variable'
ERROR tests/integration/test_harmonization.py::test_harmonization_sr15[Emissions|NH3] - KeyError: 'variable'
ERROR tests/integration/test_harmonization.py::test_harmonization_sr15[Emissions|OC] - KeyError: 'variable'
ERROR tests/integration/test_harmonization.py::test_harmonization_sr15[Emissions|SF6] - KeyError: 'variable'
ERROR tests/integration/test_harmonization.py::test_harmonization_sr15[Emissions|Sulfur] - KeyError: 'variable'
ERROR tests/integration/test_harmonization.py::test_harmonization_sr15[Emissions|NOx] - KeyError: 'variable'
ERROR tests/integration/test_harmonization.py::test_harmonization_sr15[Emissions|VOC] - KeyError: 'variable'
================== 9 failed, 139 passed, 34 deselected, 26 warnings, 24 errors in 1479.40s (0:24:39) ==================

znicholls commented 9 months ago

Potentially my previous "update-expected-files" (16a1491) may have caused some issues now that you added back in your hack, we can try reverting it?

Ah ok yep I would try reverting that then and see if it behaves. I think we should be able to do this MR without having to update the expected output (or at least I think that should be the goal as upgrading the packages shouldn't change the numbers!).

The following suggests to me that there's still a mismatch between expected and produced 'exclude' column outputs (which I didn't understand last time, because it didn't seem to be in the real outputs)

Yes this looks to me like the exclude column is now removed/added compared to what pyam used to do?

On my laptop, I now actually get different fails/errors (KeyError) messages than online, not sure whether something is up with my installation leading to different tests/files used when I do pytest tests

I suspect so. I would try deleting your virtual environment and starting again and then seeing if the error remains the same.

phackstock commented 8 months ago

@jkikstra and @znicholls are there any updates on this? Would be great to have this resolved before the end of the year. If there's anything I can do to help please just let me know :)

jkikstra commented 8 months ago

Potentially my previous "update-expected-files" (16a1491) may have caused some issues now that you added back in your hack, we can try reverting it?

Ah ok yep I would try reverting that then and see if it behaves. I think we should be able to do this MR without having to update the expected output (or at least I think that should be the goal as upgrading the packages shouldn't change the numbers!).

Indeed, The numbers shouldn't change. But I don't care much for the exclude column, which we don't use in the output anyway currently. So whether or not it is there, doesn't matter to us, which is what I hoped to resolve initially with the updating.

Now tried with a fresh conda environment (using python 3.10), but still getting these KeyError issues. I'll dig a bit deeper and try also with 3.10, since that's what the GHA workflow was running on. EDIT: I don't think it's a python version issue.

jkikstra commented 8 months ago

Hmm okay so I keep being stranded at getting this "KeyError" issue on my laptop for tests\integration\test_harmonization.py, which refers back to what I think should be a standard operation, coming just after the drop_broken_stuff() which is why last time I took that out:

src\climate_assessment\harmonization\__init__.py:333: in run_harmonization
    equiv_rows = scenarios_harmonized["variable"].isin(["Emissions|HFC"])

Besides that, there are still a few other test failures too.

So Philip, would gladly take up your offer - let's connect tomorrow if you have time (I'll write you on Teams).

jkikstra commented 8 months ago

Okay, @phackstock and I sat down. There's a few things.

Local installation issues on my Windows laptop The KeyError issue I got above is mysteriously linked to me having done pip install -e . only. It goes away once I managed to install with pip install -e .[tests,deploy,linter,notebook] We have no clue why yet. We also have no clue why I could even run pytest without it giving an error, on a freshly installed python 3.10 interpreter, in a fresh conda environment, not having installed tests and not having pytest show up when I do pip freeze. Philip suggested that perhaps conda somehow was using a global version of the package, but we also don't know how that would have worked. Philip, on Linux, could not reproduce this.

Additionally, I had to do ` instead ofpip install -e .[dev]because the latter created a conflict betweensphinxandawscli`. Again, Philip, on Linux, could not reproduce this.

Meta issue: exclude column

[x] FAIL: test_postprocess.py::test_postprocess In a recent pyam update, exclude changed from a column to an attribute. This should have affected only the places where we have a meta, so especially

A quick deleting of the column in one file indeed seems to have resolved this issue. However, it revealed a new mysterious issue, where the data columns (i.e. 1995, 1996, ..., 2100) in the newly produced data now were not integer anymore (as expected) but rather strings (i.e. '1995', '1996', ..., '2100').

Way to resolve:

[x] delete the exclude column in the test-data files where it is present
[x] find out and resolve the string/integer issue: follow pyam's integer default

Data issue: different climate outcomes

[ ] FAIL: test_workflow.py::test_workflow_ciceroscm
[ ] FAIL: test_workflow.py::test_workflow_fair
[ ] FAIL: test_clim.py::test_clim

Somehow, it looks like climate output values are different from what is expected. We see large differences in e.g. 2100 temperature in cicero (about 0.8K), and noticeable differences in fair (about 0.05K).

Way to resolve: Bit unclear still. Here's my thoughts on where to start. Because none of the climate models have changed, we would first need to check if there's an issue with the emissions input. It might be worth starting with a look at the "model9"-"1point5" pathway in ex2.csv, for either cicero or fair. I.e., using the "_alloutput.xlsx" (e.g. "ex2_harmonized_infilled_alloutput.xlsx") test-data files, look at the infilled emissions (e.g. AR6 climate diagnostics|Infilled|Emissions|CO2|Energy and Industrial Processes), and compare to what the testing pipeline produces now, before feeding into the climate models. If there's no difference in the emissions, we should worry again.

@phackstock will now take over this branch.

znicholls commented 8 months ago

Local installation issues on my Windows laptop

Yikes, nice detective work.

Meta issue: exclude column

Weird. Do whatever in terms of removing exclude and changing to int or not (it's easy enough to git diff to check that only that column changes)

If there's no difference in the emissions, we should worry again

Yes. I guess if there is a difference in the emissions, we have to fix that. If there isn't a difference in the emissions, something has changed in the post-processing. That is possible with pandas 2.0. @phackstock if you find that the emissions are in fact identical ping me and I can go diving into emulator land. If only the climate tests are failing (and all the other processing tests e.g. harmonisation and infilling related tests are passing) then this suggests to me already a problem in the emulators/post-processing, but I will wait until the emissions checks are done before diving in just in case my instinct is wrong.

phackstock commented 7 months ago

Fixed the first issue on your list @jkikstra. The issue ended up being both the extra exclude column in the reference data as well as the year column names being read in as numbers rather than strings. Let's see if I can get the remaining tests to work as well.

phackstock commented 7 months ago

Fixed the pyam.IamDataFrame.reset_index. From what I can see there's now just tow types of errors remaining:

The openscmrunner.run
The actual failing climate model tests

@znicholls you can take over here, fixing 1. should be a matter of seconds for you, fingers crossed for 2.

znicholls commented 7 months ago

Alrighty we're done. The CICERO test is failing, but that is because we fixed a bug in CICERO's heat uptake handling in Openscm-runner way back (https://github.com/openscm/openscm-runner/blob/main/docs/source/changelog.md#openscm-runner-v093-2022-01-19).

So, if you have a linux machine and want to just re-run the failing test to update it, then go for it. Otherwise, I can do that Tuesday.

As usual, a weird pandas bug was the culprit. This package is also just such a mess though, maintenance is going to be an ongoing headache.

phackstock commented 7 months ago

@znicholls thanks a lot for your work 👍 amazing that we'll now be compatible with the lastest version of the Scenario Explorer processing. If you could fix the final test tomorrow that would be great. I'll approve the PR now so that you can just go ahead with the merge afterwards. This update would probably also warrant a new release.

iiasa / climate-assessment

Update dependencies (Jarmo-fix) #50