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

Add units agnostic harmonisation #42

Merged
merged 21 commits into from
Mar 17, 2022
Merged

Conversation

znicholls
Copy link
Contributor

@znicholls znicholls commented Aug 29, 2021

Add convenience functions which make it simpler to harmonise a number of timeseries and can also handle mismatches in units.

There's a few problems this is aiming to solve:

  1. There's no obvious interface that would just work with the output of iamdf.timeseries() (maybe I've missed something)
  2. The fact that there's not an obvious interface to use if you just had a single timeseries you wanted to harmonise. At the moment, you'd need to instantiate a harmoniser, make sure the rc configurations are set right (maybe, depending on which class you started with) etc. and then call harmonise before extracting the output. This new convenience function lets you just throw in the timeseries to harmonise, the history and the harmonisation year and returns you the output.
  3. Related to the above, if you want to harmonise lots of scenarios, you have to write your own loops to go through each model and scenario combination. This interface handles all that for you.
  4. The current interfaces explode if the units aren't just right (CO2 has to be in MtCO2 / yr, there's a whole bunch of gases which are expected to be in kt etc.). The new interface handles unit conversion (although it's a bit clunky cause pyam/scmdata aren't dependencies, if they were it could be even simpler again).

All I've done is add a single module, convenience, with a single function, harmonise_all, that addresses the issues above. The new tutorial notebook gives an illustration of how to use it and demonstrates a) how it can handle unit conversion and b) how the default decision tree can be overridden.

The changes in methods are to handle the time columns being integers rather than strings. They don't break any other tests so I think they're written in a suitably flexible way. I also picked up a dependence on the gas in the default decision tree, but this appears to be undocumented. I can make a separate issue for that if helpful.

@znicholls
Copy link
Contributor Author

@lewisjared fyi

@znicholls znicholls marked this pull request as ready for review August 30, 2021 05:19
@znicholls
Copy link
Contributor Author

@jkikstra or @gidden this is a pretty big PR. I'm happy to split it into pieces if helpful, but the general approach is hopefully fairly clear if you look at the documentation?

@gidden
Copy link
Member

gidden commented Aug 30, 2021

Hey @znicholls I can try to get to this in the next day or so. It would be useful to add a bit more context and flavor to the description which would help me understand the motivation (what didn't work) and solution.

@znicholls
Copy link
Contributor Author

I can try to get to this in the next day or so

Awesome thanks. There's not a massive rush unless @jkikstra wants to simplify the WG3 process by using something like this.

It would be useful to add a bit more context and flavor to the description which would help me understand the motivation (what didn't work) and solution.

No worries, done


def _harmonise_single(timeseries, history, harmonisation_year, overrides):
assert timeseries.shape[0] == 1
# unclear why we don't use pyam or scmdata for filtering
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
# unclear why we don't use pyam or scmdata for filtering
# unclear why we don't have pyam or scmdata as a dependency as this would simplify filtering and other handling

@gidden
Copy link
Member

gidden commented Sep 1, 2021

Hi @znicholls , starting to look at this now. It's a great addition, and if it's ok for you and @lewisjared and @jkikstra I'd like to try to take a deeper look to see how much of this can be incorporated in the guts of aneris.

So if it's ok to delay a detailed review for sometime, that'd be great.

To answer a lot of your questions very simply, aneris predates pyam, so did not use it of course..

@znicholls
Copy link
Contributor Author

znicholls commented Sep 1, 2021 via email

@gidden
Copy link
Member

gidden commented Mar 17, 2022

Hey @znicholls - picking this up again so WG3 can get out the door. I have not had time to do any more robust thinking/coding on this. Accordingly, I suggest the following way forward:

  1. I will restart tests so we make sure nothing has affected them in the intervening time
  2. can you @znicholls make an issue describing what would be needed for migrating these improvements into the body of aneris please?
  3. once both are complete, I'll merge

@znicholls
Copy link
Contributor Author

2. can you @znicholls make an issue describing what would be needed for migrating these improvements into the body of aneris please?

Brain dump in #46. Perhaps not as clear as we would hope but it should get to the issues (although you might have to wade through some waffle to get there)

@gidden
Copy link
Member

gidden commented Mar 17, 2022

Thanks @znicholls !

@gidden gidden merged commit 6d81bb1 into iiasa:master Mar 17, 2022
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.

2 participants