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

Ar6 colors #213

Merged
merged 28 commits into from Apr 15, 2019
Merged

Ar6 colors #213

merged 28 commits into from Apr 15, 2019

Conversation

gidden
Copy link
Member

@gidden gidden commented Apr 4, 2019

User-configuration support for AR6 color palate

TO NOTE I had to update our plotting CI to use a newer freetype version. This will cause necessary updates to our CI as well..

  • Tests Added
  • Documentation Added
  • Description in RELEASE_NOTES.md Added

image

We now can set our run_control to say we want specific AR6 scenario colors. Need to add docs, etc., but let me know what you think about color names, etc. You can see an example in the updated test. Basically you need a config like:

color:
  ssp-type:
    SSP3-7.0: "AR6-SSP3-7.0"
    History: black
    # and so on

pyam/plotting.py Outdated Show resolved Hide resolved
pyam/plotting.py Outdated Show resolved Hide resolved
tests/test_plotting.py Show resolved Hide resolved
tests/test_plotting.py Outdated Show resolved Hide resolved
@gidden gidden requested a review from znicholls April 4, 2019 14:13
@gidden
Copy link
Member Author

gidden commented Apr 4, 2019

Ooof, thanks stickler.. you can tell I must have just moved to a new laptop without the exact same emacs config..

@coveralls
Copy link

coveralls commented Apr 4, 2019

Coverage Status

Coverage decreased (-0.02%) to 91.38% when pulling 39df3c1 on gidden:ar6-colors into d24cbb7 on IAMconsortium:master.

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.

Looks good, pending working out CI stuff. Do you want to do the CI update on this PR, then update all others, or push the other PRs through first, then update the CI (keeping in mind that I think #198 has a nice way to split things out so environment management for CI and local testing is easier and clearer)?

pyam/plotting.py Show resolved Hide resolved
pyam/plotting.py Show resolved Hide resolved
@danielhuppmann
Copy link
Member

thanks @gidden, great that we are moving towards AR6 colors!

But echoing @znicholls question, I'm not sure I understand how this is expected to work in practice. If the scenario name has to contain the project color coding (like scenario='AR6-SSP5-8.5'), I don't think that's a good direction.

Rather, I'd go for something like pyam.plotting.set_color_palette('AR6') and the have scenario names more "natural" (scenario='SSP5-8.5'). This would also make forward/backward compatibility in the future much easier.

@znicholls
Copy link
Collaborator

Rather, I'd go for something like pyam.plotting.set_color_palette('AR6') and the have scenario names more "natural" (scenario='SSP5-8.5'). This would also make forward/backward compatibility in the future much easier.

I'd actually disagree with this. One person's 'natural' name for the SSPs is another one's unnatural naming. I think a very specific naming scheme like you have is ok, as long as the tutorial shows clearly how to set the colours for whatever scheme one wants e.g. if your convention is "SSP585", do this, if it's "SSP5-8.5", do this, if it's "RCP26-SSP1", do this.

@danielhuppmann
Copy link
Member

One person's 'natural' name for the SSPs is another one's unnatural naming.

Agree, @znicholls, that there is no one unique naming convention - but it should not be necessary to encode the color palette in the scenario name.

@znicholls
Copy link
Collaborator

Agree, @znicholls, that there is no one unique naming convention - but it should not be necessary to encode the color palette in the scenario name.

You have to put the information about where the colour palette comes from somewhere? Would you instead have PYAM_COLOURS be a dictionary so you could have e.g. PYAM_COLOURS["AR6"]["SSP585"]?

@danielhuppmann
Copy link
Member

Yes, a dictionary or loading a specific config file based on pyam.plotting.set_color_palette('AR6') would be ideal.

@gidden
Copy link
Member Author

gidden commented Apr 5, 2019

Hi all, great discussion so far. I should note that this is just one suggestion of an initial implementation that we can build off of in the future.

The way that it works right now is to recognize certain color names and only in the run_control(). So you can say

run_control().update({'color': {'model': {'my-special-model-name': 'AR6-SSP2-4.5'}}})

As implemented, the dictionary is just like saying color='blue', except you get to say color='AR6-SSP2-4.5'. In my current setup for the WG1 figure, in fact I set a new metadata column called ar6-type, assign the model/scenario appropriately there, and then use color='ar6-type'.

I say that this is preliminary because there could be other, "smarter" was to do this, e.g., by trying to develop a cmap or something similar. However, there is so much that has to be "intuited" by that approach, I thought it would be simpler and more user-friendly to just get the users to tell us which color to use (much like we already do throughout pyam). We just now recognize certain specific color names and map them to specific RGB values.

If someone wants to try to implement (or at least propose an implementation) a color-palate-setting approach, I'd be all ears! I thought about it for a while and this was the conclusion I came to.

To note, we could break this PYAM_COLORS dictionary into further key-value pairs (e.g., AR6, AR5, etc.). However that would then need special logic in the look up. For example, you could say the "color name" is AR6:SSP2-4.5 and then do something like:

parts = color.split(':')
if parts[0] in PYAM_COLORS:
    color = PYAM_COLORS[parts[0]][':'.join(parts[1:])

But I'm not sure what the win is here over just naming it explicitly.

@gidden
Copy link
Member Author

gidden commented Apr 5, 2019

@znicholls regarding #198, I think we still have a bit to do on the review there. is it easy to break out the commits related to CI and issue a separate PR? Or are they too intricately connected to do so?

@gidden
Copy link
Member Author

gidden commented Apr 5, 2019

Ok took a quick look and I think it's best to wait here for CI discussion on #198 either by merging that first or breaking out the CI components and merging that smaller PR first.

@znicholls
Copy link
Collaborator

is it easy to break out the commits related to CI and issue a separate PR?

Yep will do so now

@znicholls
Copy link
Collaborator

Ok let's look at #214 first then go from there

@gidden
Copy link
Member Author

gidden commented Apr 9, 2019

Ok folks, after much pain, we now have updated CI on all platforms. As part of this, I have updated a bit of the Makefile infrastructure - @znicholls can you take a look and make sure all is a-ok?

I also found that we are now running into our depedencies dropping python 2 support. A number of them require python3 to install appropriate versions on conda, so I have made the executive decision to drop our python2 CI instead of having to completely regenerate/copy it and keep copies of all expected images. Let me know if anyone really thinks this is a bad idea, but I think it's our best pathway forward.

I also expanded a bit the python versions tested - 3.6 and 3.7 are tested on all 3 OSes. I tried python3.5, but it ran into the same issues (conda quietly upgraded to python3.7 because of some dependency we have).

@gidden
Copy link
Member Author

gidden commented Apr 9, 2019

But in any case, @danielhuppmann and @znicholls, let's get back to the more important discussion of how best to implement the colors!

@znicholls
Copy link
Collaborator

znicholls commented Apr 9, 2019

@znicholls can you take a look and make sure all is a-ok?

lgtm, will put minor tweaks in a PR

I have made the executive decision to drop our python2 CI

Good call from my end

But I'm not sure what the win is here over just naming it explicitly.

I agree with this so have only put addition of RCP colours in my PR

* Add RCP colours and tweak Makefile

* Update naming convention
pyam/plotting.py Outdated Show resolved Hide resolved
@gidden
Copy link
Member Author

gidden commented Apr 10, 2019

Ok everyone, thanks to @znicholls, we now have support also for the AR5 color palate. In addition to the existing implementation, I will add a short tutorial on how to use it. Are we happy with the implementation as is? Do we want to revisit the single key-value pair strategy for colors?

@gidden
Copy link
Member Author

gidden commented Apr 10, 2019

Hey @znicholls. CI is running into the same issue as I did on my local machine. Namely the line here causes make to fail (see here). I will remove it again but just want to make sure you're aware.

@gidden
Copy link
Member Author

gidden commented Apr 10, 2019

Or I guess now it is a bit different than what I saw implementation wise, but the result is the same (nothing to do for target)

@gidden
Copy link
Member Author

gidden commented Apr 10, 2019

FYI @znicholls I had to revert the makefile changes, not sure what about them caused CI to error, but if you're interested, would you mind opening up a new pr with those changes to investigate?

@znicholls
Copy link
Collaborator

would you mind opening up a new pr with those changes to investigate?

yep see #218

* Try updating make setup for CI

* Get venv dependencies right

* Update doc requirements for Makefile
@gidden
Copy link
Member Author

gidden commented Apr 11, 2019

@znicholls, @danielhuppmann: #198 is now being built on this PR, which makes review there complicated. Are we ok to pull this in now and making an issue to add a tutorial for it's use (since I can't get to that immediately)? That would allow me to then review #198. Thoughts?

@znicholls
Copy link
Collaborator

That would allow me to then review #198. Thoughts?

You could also review in gidden#8 instead, should make it simpler?

@gidden
Copy link
Member Author

gidden commented Apr 11, 2019 via email

@danielhuppmann
Copy link
Member

I think it's fine to pull this PR (affect updating the readme and docs, but that should only take a minute). Thanks @gidden for making pyam AR6-compatible!

Copy link
Member

@danielhuppmann danielhuppmann left a comment

Choose a reason for hiding this comment

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

lgtm, questions about usage have been resolved in offline discussions.

@znicholls
Copy link
Collaborator

lgtm too @gidden, I'll leave it for @danielhuppmann to merge overnight once the README etc. are good to go

@gidden
Copy link
Member Author

gidden commented Apr 11, 2019

Ok @danielhuppmann tutorial is added and this should be good to merge after CI passes

@danielhuppmann danielhuppmann merged commit 3972e8c into IAMconsortium:master Apr 15, 2019
@gidden gidden deleted the ar6-colors branch June 15, 2022 11:28
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.

None yet

5 participants