Additional testcases - Githubissues

iiasa / aneris

Harmonization of Emissions Trajectories for Integrated Assessment Models

Apache License 2.0

4 stars 11 forks source link

Additional testcases #18

Closed jkikstra closed 5 years ago

jkikstra commented 5 years ago

Based on this suggestion by @gidden in #15, three additional test cases are introduced in this PR.

Note: the test_data for the regression tests has not been quality-checked/validated (@gidden this is for you to do as an expert on how the harmonization should behave). No overrides are yet provided.

Issue: currently, with the binary switch global_harmonization_only, only a subset of the emissions gets harmonized (i.e. only global aggregates or no global aggregates) -- this needs a fix.

gidden commented 5 years ago

@jkikstra I rebased this onto current master so we only see the commits related to this PR

gidden commented 5 years ago

Hey @jkikstra this PR now contains passing tests for all cases you provided - I had to do a bit of revision to the test cases, but the basic idea is as follows: for each trajectory defined at region and sector level, apply a simple to compute override (constant_ratio and constant_offset). This guarantees that the aggregation also works as expected.

I additionally added features for:

returning directly the objects, see https://github.com/iiasa/aneris/pull/18/files#diff-3437ae6214d4c2f163341d9e5e0853f5R43
providing a custom suffix, see: https://github.com/iiasa/aneris/pull/18/files#diff-9c03adcf85f191a2de5ff1c6792223f4R4

I think this is the barebones you need to complete the AR6 setup. Once I find infinite time, these things should be better documented, but, sadly, that day is not today.

Let's catch up again this week to confirm that you have everything you need here.

gidden commented 5 years ago

thanks @jkikstra - ignore the stickler issues for now, its the first time this repo is being checked this way

gidden commented 5 years ago

Ok @jkikstra this PR now holds only the edits you're working on at the moment so we can more easily identify changes!

gidden commented 5 years ago

hey @jkikstra - turns out check_null() was in fact a canary in the coal mine here. Here's what was happening:

check_null() (yes, poorly named perhaps) checks if any null values exist and drops (removes) them
the drop happens by default row-wise (axis=0)
null values (i.e., missing data) existed in the test datasets - notably 2005/2010 data for certain F-gases
therefore, check_null() dropped those rows
therefore, it appeared like the model was actually missing those data (if you look at your test file, they all have constant values, and if you look at the metadata, it says model_zero)

I have updated this so that the drop happens column wise (axis=1). This is probably better for our use case, but, if a team presents results with missing data for one gas in a column, that whole column would go away.

I think it is probably ok to leave this as is for now, but perhaps it is worth discussing with Ed how to deal with missing data before sending it to aneris

gidden commented 5 years ago

Note please that the test file will need to be updated with correct expected values. Once that is done, we can merge the whole chain.

jkikstra commented 5 years ago

Great, thanks as lot @gidden for finding the issue here!

That's a good point you raise. For what we have encountered now:

deleting rows (gases) when the missing data is before the harmonization year is unwanted and unnecessary, deleting a column (year) would not be a problem

In other cases:

however, a missing value in the harmonization year should probably lead to deleting the row (the GHG), as (in my understanding -- or is there interpolation that could be done?) the gas currently could not be harmonized anyway, and deleting the row would lead to no harmonization taking place for any gas.
with missing values after the harmonization year, it is a question of design, and I do not know what would be preferred. Deleting the column would only make sense if the model does not report any gases in these years (which is a likely case, e.g. models reporting only up till 2050). Deleting rows makes more sense if only pathways fully reporting up till 2100 should be taken into account. If we would move away from check_null(), other options would open up, such as building in a check that just harmonizes up until the year that has been reported for a certain gas.

For our use case, I'm discussing this here with Ed, we'll do some pre-processing/checking before feeding scenarios into aneris.

jkikstra commented 5 years ago

Hi @gidden, I'll go ahead and merge this as I've now updated the test file and everything passes.