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

Bump pyam-iamc to >= 1.9.0 #36

Merged
merged 6 commits into from
Aug 4, 2023
Merged

Bump pyam-iamc to >= 1.9.0 #36

merged 6 commits into from
Aug 4, 2023

Conversation

phackstock
Copy link
Contributor

@phackstock phackstock commented May 23, 2023

closes #27.

  • Tests added
  • Documentation added
  • Example added (in the documentation, to an existing notebook, or in a new notebook)
  • Description in CHANGELOG.rst added (single line such as: (`#XX <https://github.com/iiasa/climate-assessment/pull/XX>`_) Added feature which does something)

@jkikstra jkikstra mentioned this pull request May 23, 2023
@phackstock
Copy link
Contributor Author

Looks like there's four tests that are breaking. If any of you @znicholls, @lewisjared or @jkikstra could have a quick look that would be great.

@znicholls
Copy link
Collaborator

I think I worked it out. Put the code below after line 360 of checks.py and you'll see. I think the original regression is in pandas, but you can work around it by tweaking pyam

        if model == "model7":
            import pdb
            pdb.set_trace()
            df_scen.timeseries()  # No CO2|Energy and Industrial as expected
            df_scen._data.index.get_level_values("variable").unique()  # No CO2|Energy and Industrial as expected

            # Using pyam accessors gives wrong result so `has_co2_energy`
            # evaluates incorrectly above
            df_scen.variable  # Contains CO2|Energy and Industrial ?!

            # What pyam uses internally. I think pyam is correct, but pandas
            # has a crazy regression where this returns incorrect information?!
            list(df_scen._data.index.levels[df_scen._data.index._get_level_number("variable")])  # Contains CO2|Energy and Industrial ?!
            # I assume that accessing levels directly on a MultiIndex is somehow not the intended pattern

            # I'd suggest updating pyam's internal's to use this, which seems to work
            list(df_scen._data.index.get_level_values("variable").unique())  # No CO2|Energy and Industrial as expected

@phackstock
Copy link
Contributor Author

Thanks @znicholls, that's super weird. I've implemented your suggestion.
Is this a pyam bug? Could it be related to a variable being dropped and df.variable not being updated accordingly?

znicholls
znicholls previously approved these changes May 24, 2023
Copy link
Collaborator

@znicholls znicholls left a comment

Choose a reason for hiding this comment

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

I think once the change is applied globally this should pass through. Option b is you do a bugfix release in pyam (fixing the pandas bug), do a new pyam release then change this MR and you don't have to do any fix at all here.

@@ -369,7 +369,7 @@ def check_reported_co2(df, filename, output_csv=False, outdir="output"):
elif has_co2_total and has_co2_afolu:
df_scen = _check_difference(df_scen, co2_total, co2_afolu, scen, model)

if co2_energy not in df_scen.variable:
if co2_energy not in df_scen._data.index.get_level_values("variable").unique():
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you'll need to make this replacement everywhere that df_scen.variable appears to get things to work properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah right, of course

@phackstock
Copy link
Contributor Author

Option b is you do a bugfix release in pyam (fixing the pandas bug), do a new pyam release then change this MR and you don't have to do any fix at all here.

So you think it's a pandas bug?
Not sure what's the best option here, if you can reproduce it, maybe open a pyam issue and we discuss over there? It's not super critical to update pyam for climate-assessment.
We could do #34 now then and just increase the pyam version later.

@danielhuppmann
Copy link
Member

So I"m trying to get to the bottom of this, but I can't replicate the observation by @znicholls about df.variable returning something else than the correct values (with pyam 1.9 and pandas 1.5.3)... It seems all correct to me.

@danielhuppmann
Copy link
Member

So I ran the tests along the entire history of pyam and it seems that IAMconsortium/pyam#731 breaks the test.

@znicholls
Copy link
Collaborator

pandas 1.5.3

I think I had pandas 2 maybe (memory fading now)

I ran the tests along the entire history of pyam

Nice detective work, did you use git bisect?

@znicholls znicholls mentioned this pull request Aug 3, 2023
@znicholls
Copy link
Collaborator

(I've made a bit of a mess of this, sorry)

@danielhuppmann
Copy link
Member

Implement a fix based on @znicholls' observation in IAMconsortium/pyam#762

@phackstock
Copy link
Contributor Author

After the pyam update, I wanted to update this PR to verify that that fixes the issue. However, as I just saw the tests were already passing yesterday? I'm a bit confused now but I guess it's all good?

@danielhuppmann
Copy link
Member

See ccf0e7b - the problem was caused by the very inefficient groupby, so I refactored that piece of the code.

@phackstock phackstock self-assigned this Aug 4, 2023
@phackstock
Copy link
Contributor Author

Since the tests are passing and I'd love to get this out before the weekend, I'll move on with the merge. Hope that's ok @znicholls.

@phackstock phackstock merged commit 9b701da into main Aug 4, 2023
3 checks passed
@phackstock phackstock deleted the update/pyam-iamc branch August 4, 2023 11:19
@znicholls
Copy link
Collaborator

Hope that's ok @znicholls.

If all passing, all good (particularly given how much we've talked about this already)

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.

Bump pyam-iamc to 1.8.0
3 participants