init sankey - Githubissues

mabudz commented 8 months ago

Create sankey diagram and add tutorial.

The approach here creates the sankey diagram by using the sankey plot function of the pyam package. This function requires a mapping dictionary. The mapping dict is created by using the newly created automatic report message::sankey and a utility function ~sankey_mapper()~map_for_sankey.

This pull request is related to the draft pull request in message_data , which can be removed, since the sankey diagram has been implemented in the pyam package.

[!NOTE] The new key is now only added when calling Reporter.add_sankey().

How to review

General approach should be reviewed. Also, I am not sure if the util function ~sankey_mapper()~ map_for_sankey() is at the right place.

PR checklist

[x] Continuous integration checks all ✅
[x] Add or expand tests; coverage checks both ✅
[x] Add, expand, or update documentation.
[x] Update release notes.

CLAassistant commented 8 months ago

All committers have signed the CLA.

glatterf42 commented 8 months ago

Thanks for this PR :) Getting the tests to pass is a bit tricky since the API for pyam changed when they went from version 1.9.0 to 2.0.0 (as you might expect) and our code needs to satisfy both versions since we test on Python <= 3.9, which is not supported for pyam 2.0.0. So let me take a closer look, maybe we'd want to use the function that pyam's sankey function wraps around directly, after all.

glatterf42 commented 6 months ago

Thanks @daymontas1 for stepping up here, much appreciated :)

You should be able to test the PR as is and see how you like it's usefulness, but there are a few things that I would like to see happen before we merge it:

Please rebase on main to keep it up to date with current developments.
Please migrate the main functionality from util/__init__.py to somewhere more appropriate. E.g. report/sankey.py if it's a reporting tool or util/sankey.py if there are more general use cases.
Please check if we can use plotly directly instead of pyam's wrapper functionality. This should allow us to utilize more design options and avoids some dependency constraints (see above). See some examples and their docs.
Please confirm that we want to integrate the sankey functionality in the reporting as it currently stands. I'd probably ask at least @khaeru for his opinion.
Please write a test if possible to confirm that sankey_mapper maps the correct values to the correct keys. We might also need to adapt one or two reporting tests that check the number of reporting steps if this stays in the default reporting workflow.

Please feel free to ask about any of these steps if you need help :)

gidden commented 6 months ago

Hi all, to simplify here, I would simply limit this functionality to pyam > 2 - we can do a version check for this on the fly and raise an error.

glatterf42 commented 6 months ago

Rebased onto current main and expanded the PR a bit :) Thanks for jumping in here, @gidden :) @daymontas1, please pull these latest changes before making further edits. If you already have some edits locally, you can git stash them, git pull these changes and git stash pop your work on top of this.

The missing type hints (from the failing code quality check) probably indicate that we need to add pyam.* to our mypy config exclude list.

glatterf42 commented 6 months ago

The other main error I'm seeing just indicates that by adding this reporter step, we need to adapt the expected value of reporter steps in another test, which is no problem :)

glatterf42 commented 4 months ago

Is there any update here, @daymontas1? Since we want to publish a new release still this week (most likely), I'll postpone this to the 3.10 milestone.

daymontas1 commented 4 months ago

Hi @glatterf42, I tried to push the changes, but I didn't have permission to do so. So, you need to grant me access. This was my latest comment. Thanks.

glatterf42 commented 4 months ago

Alright, that's good to know. @mabudz, could you please give @daymontas1 write access to your fork? You can do that under Settings -> Collaborators and Teams -> Add people. If you do that, please confirm here so we know we can proceed.

If you don't want to wait, @daymontas1, please consider pushing your changes to a branch on your fork or on the main repository: git push -u <name of the repo> <name of the branch>. So you need to pick a repo name from the list produced by git remote -v and can choose any branch name.

mabudz commented 4 months ago

Hi @glatterf42 and @daymontas1 ,

i have added @daymontas1 to the repo ☺️

glatterf42 commented 4 months ago

Thanks for the quick reaction, much appreciated :)

khaeru commented 4 months ago

FYI all, see the GitHub docs about the “Allow edits from maintainers” option. AFAIK this is on by default, so an equivalent solution would be to add @daymontas1 to the team @iiasa/messageix-devs. Since that team has (at least) "maintainer" permissions on this repo, he would then have write access to the specific branch used in this PR (so long as @mabudz left the default value for the above option).

This avoids the work of having to (maybe) add every maintainer to every contributor's fork.

glatterf42 commented 4 months ago

Thanks, I've also added @daymontas1 to @iiasa/messageix-devs :)

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 95.6%. Comparing base (ef66e37) to head (30d8d32). Report is 20 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #770 +/- ## ======================================= - Coverage 95.6% 95.6% -0.1% ======================================= Files 46 47 +1 Lines 4344 4383 +39 ======================================= + Hits 4154 4191 +37 - Misses 190 192 +2 ``` | [Files with missing lines](https://app.codecov.io/gh/iiasa/message_ix/pull/770?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iiasa) | Coverage Δ | | |---|---|---| | [message\_ix/report/\_\_init\_\_.py](https://app.codecov.io/gh/iiasa/message_ix/pull/770?src=pr&el=tree&filepath=message_ix%2Freport%2F__init__.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iiasa#diff-bWVzc2FnZV9peC9yZXBvcnQvX19pbml0X18ucHk=) | `100.0% <100.0%> (ø)` | | | [message\_ix/tests/test\_report.py](https://app.codecov.io/gh/iiasa/message_ix/pull/770?src=pr&el=tree&filepath=message_ix%2Ftests%2Ftest_report.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iiasa#diff-bWVzc2FnZV9peC90ZXN0cy90ZXN0X3JlcG9ydC5weQ==) | `100.0% <100.0%> (ø)` | | | [message\_ix/tests/test\_tutorials.py](https://app.codecov.io/gh/iiasa/message_ix/pull/770?src=pr&el=tree&filepath=message_ix%2Ftests%2Ftest_tutorials.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iiasa#diff-bWVzc2FnZV9peC90ZXN0cy90ZXN0X3R1dG9yaWFscy5weQ==) | `97.6% <ø> (ø)` | | | [message\_ix/tests/test\_util.py](https://app.codecov.io/gh/iiasa/message_ix/pull/770?src=pr&el=tree&filepath=message_ix%2Ftests%2Ftest_util.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iiasa#diff-bWVzc2FnZV9peC90ZXN0cy90ZXN0X3V0aWwucHk=) | `100.0% <100.0%> (ø)` | | | [message\_ix/util/sankey.py](https://app.codecov.io/gh/iiasa/message_ix/pull/770?src=pr&el=tree&filepath=message_ix%2Futil%2Fsankey.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iiasa#diff-bWVzc2FnZV9peC91dGlsL3NhbmtleS5weQ==) | `100.0% <100.0%> (ø)` | | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/iiasa/message_ix/pull/770/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iiasa)

iiasa / message_ix

init sankey #770

How to review

PR checklist

Codecov Report