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

Global only #15

Merged
merged 7 commits into from
Nov 6, 2019
Merged

Global only #15

merged 7 commits into from
Nov 6, 2019

Conversation

gidden
Copy link
Member

@gidden gidden commented Nov 4, 2019

This is a branch to test and fix the issue with global-only trajectories

@gidden gidden requested a review from jkikstra November 4, 2019 17:51
@gidden
Copy link
Member Author

gidden commented Nov 4, 2019

ok @jkikstra I think this will do us for now. I've added a switch in the run control config block which states that only global trajectories will be harmonized (see e.g. https://github.com/iiasa/aneris/pull/15/files#diff-17d29a834d22c5ba2cad7b562a2df072)

This will do two things:

  1. ignore the "global only gases" put in place for CMIP6, working part of the way (but not at all close to all of the way) to make gas categories configurable #6
  2. skip the regional portion of harmonization

Please test it out and if it fits your needs and passes CI, then feel free to merge (I gave you write permissions here).

I think in the future we will want more detail in the configuration, but this should get us over the first hurdle here.

@gidden
Copy link
Member Author

gidden commented Nov 4, 2019

NOTE I have not tested this with a sectoral breakdown. Would you like to add that as another test here following the pattern I started in this PR?

@jkikstra
Copy link
Collaborator

jkikstra commented Nov 5, 2019

@gidden I went through the changes and quickly tested it to flag some stuff before I have time to write the test.

  1. in the case of global data only, now everything runs and there is harmonized output; great.
    a. Unfortunately, the AssertionError is unfortunately still present though (see below)
    b. also, not necessarily a problem, but also not super neat, is that the harmonized output now gives the same numbers twice (as it does not have an elegant way to deal with species data that is not composed of sectors, and will therefore make an extra "total" even though there is only one figure).
File "c:\users\kikstra\documents\github\aneris\aneris\cli.py", line 64, in harmonize
    driver.harmonize(scenario)
  File "c:\users\kikstra\documents\github\aneris\aneris\harmonize.py", line 476, in harmonize
    scenario).results()
  File "c:\users\kikstra\documents\github\aneris\aneris\harmonize.py", line 353, in process
    self._downselect_var()  # only prefix|*|suffix
  File "c:\users\kikstra\documents\github\aneris\aneris\harmonize.py", line 297, in _downselect_var
    assert(len(self.hist) > 0)
AssertionError```

@gidden
Copy link
Member Author

gidden commented Nov 5, 2019

@jkikstra the test I implemented should be extendable to at least all non-CO2 gases in the AR6 use case. Is the issue you're seeing related to CO2?

On the multiple entries, I don't see this in the test output here

I can really only implement things at night for the moment, so if you can write tests that fail, I can try to develop patches for them or identify how to avoid the behavior.

@gidden
Copy link
Member Author

gidden commented Nov 5, 2019

Hey @jkikstra, would you mind opening new PRs with the tests that are expected to fail? Apologies, should have been more clear on that.

* add test: global-sector (one gas)

* remove extraneous, update with overrides
@gidden
Copy link
Member Author

gidden commented Nov 5, 2019

hey @jkikstra so this takes care of two cases, but unfortunately, we have the final one, which is a combination (multiple co2 global sectors, all other global totals). This unfortunately does not work out of the box and I need to think of the best way to approach it.

@gidden
Copy link
Member Author

gidden commented Nov 6, 2019

Hey @jkikstra, when you have time before the end of the week, could you please make a PR with additional test cases into this branch? I will work on this either on Friday on the weekend - I'm basically booked until then.

All tests can be very simple with a single historical year (e.g., 2005) and two model years (e.g., 2005/2010).

Test 1: a mock of what we need for the pipeline prototype

  • only global
  • one gas (e.g., BC) split between two sectors
  • another gas (e.g. Sulfur) with a total trajectory

Test 2: on the way to future needs

  • same as test 1 for first two gases
  • an additional gas (e.g., OC) defined for two regions, only sector total

Test 3: full potential functionality for AR6

  • same as test 2 for first 3 gases
  • an additional gas (e.g., CH4) defined for two regions and two sectors

Let's chat to clarify as needed. Hopefully, we'll be done with code updates after this =)

@jkikstra
Copy link
Collaborator

jkikstra commented Nov 6, 2019

hey @jkikstra so this takes care of two cases, but unfortunately, we have the final one, which is a combination (multiple co2 global sectors, all other global totals). This unfortunately does not work out of the box and I need to think of the best way to approach it.

just for reference, this point should be addressed by PR #17
Admittedly, the test in PR #17 is not as simple as it could/should be.

@gidden On your question on writing there 3 minimal tests: yes, I will make sure to put out a pull request with these three before Friday.

@gidden
Copy link
Member Author

gidden commented Nov 6, 2019

Sounds good. I will go ahead and merge this PR so it doesn't get too large. Please open the new PRs into master (at least that's the plan for the moment!

@gidden gidden merged commit 7fcc0f5 into master Nov 6, 2019
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