RMI-PACTA / pacta.portfolio.report

pacta.portfolio.report
https://rmi-pacta.github.io/pacta.portfolio.report/
MIT License
1 stars 0 forks source link

add legend ordering for new scenarios in `trajectory_alignment.js` #54

Closed cjyetman closed 5 months ago

cjyetman commented 5 months ago
MonikaFu commented 5 months ago

@cjyetman thanks! I will give it a look tomorrow morning!

MonikaFu commented 5 months ago

@cjyetman I run the code and looked into the report generated and the problem seems still not to be resolved. I looked into the .js just to make sure that I am using the code from this PR and I am using the same version but the plots don't render correctly. For example, for ISF there are no scenarios

image

And for WEO2023 we see a black area while there should only be colourful scenario areas.

image
jdhoffa commented 5 months ago

@cjyetman I run the code and looked into the report generated and the problem seems still not to be resolved. I looked into the .js just to make sure that I am using the code from this PR and I am using the same version but the plots don't render correctly. For example, for ISF there are no scenarios

image

And for WEO2023 we see a black area while there should only be colourful scenario areas.

image

@cjyetman just jumping in here, I could be wrong, but I believe this is because we need to re-run data-prep to pick up the changes in scenario name in pacta.scenario.preparation? At least for the case of WEO?

ISF I'm not sure what the issue is

cjyetman commented 5 months ago

I did advise @MonikaFu that the WEO NZE scenario needs to be temporarily, manually adjusted to WEO_NZE instead of WEO_NZE_2050 in the workflow.transition.monitor project configs to work with the current data.prep outputs that we have. That appears to be the problem in these plots because NZE is missing. I also don't know if the explicitly defined "worse" is actually necessary, I just tried to follow the previous pattern. My expectation was that "worse" defined the color of the area beyond the worse scenario, but I don't really know how this works.

With ISF2023, I also don't know if it really needs "worse". Also no idea why it's not picking up "1.5°C".

If this doesn't work, we can close this ticket and I guess @MonikaFu will have to figure it out.

MonikaFu commented 5 months ago

I have adjusted the parameter as advised but still see this issue. If I look into the parameters used in the code I see select_scenario = "WEO2023_NZE" which is what we want with current data.prep outputs right?

You always need "worse" as far as I remember in the way the colour assignment works in this plot. But would need to dive deeper to recall.

Yes, I guess since it seems this PR does not manage to solve the issues with adding new scenarios I probably need to take over.

jdhoffa commented 5 months ago

Then is the intention to close this PR?

MonikaFu commented 5 months ago

Then is the intention to close this PR?

I'm working within this PR as the changes @cjyetman made are needed but there is more that needs to change for the scenarios to be picked up.

jdhoffa commented 5 months ago

@MonikaFu is this ready for review? or was the review request a mistake?

MonikaFu commented 5 months ago

@MonikaFu is this ready for review? or was the review request a mistake?

it was kind of a mistake. I did not want it to have the 'approved' status so I re-requested a review but I only realised after that it will ask you for a review of course. It is not ready for review that is why I put it into 'draft' mode.

MonikaFu commented 5 months ago

@jdhoffa I believe this is solved now. The 'WEO2023' source still does not work properly because the plot expects a scenario called 'NZE_2050' but in the current version of the data it is 'NZE'. From what I understand this is going to be fixed and the proper name should be 'NZE_2050' so I am not going to change it.

MonikaFu commented 5 months ago

@cjyetman maybe you would like to have a look at it as well?

cjyetman commented 5 months ago

Seems to function as expected, minus the NZE_2050 situation. But... is "worse" supposed to appear black? and is there something wrong with the order of "worse"?

Screenshot 2024-04-05 at 13 44 08 Screenshot 2024-04-05 at 13 44 21

also, not sure if this is related, but I get 2 JavaScript errors that prevent the trajectory plots from displaying until I change one of the selectors...

Screenshot 2024-04-05 at 13 46 11 Screenshot 2024-04-05 at 13 46 25
jdhoffa commented 5 months ago

Seems to function as expected, minus the NZE_2050 situation. But... is "worse" supposed to appear black? and is there something wrong with the order of "worse"? Screenshot 2024-04-05 at 13 44 08 Screenshot 2024-04-05 at 13 44 21

also, not sure if this is related, but I get 2 JavaScript errors that prevent the trajectory plots from displaying until I change one of the selectors... Screenshot 2024-04-05 at 13 46 11 Screenshot 2024-04-05 at 13 46 25

Is this an artifact of the NZE_2050 issue (and a mismatch between expected and actual scenarios?)

Does "worse" still appear black if you try a different scenario?

MonikaFu commented 5 months ago

Worse should not appear at all but it appears because of the mismatch between expected and actual scenario names. No idea about the errors. I don't think I was getting any but can look into it again.

cjyetman commented 5 months ago

Is this an artifact of the NZE_2050 issue (and a mismatch between expected and actual scenarios?)

Does "worse" still appear black if you try a different scenario?

I'm not totally sure, too many hoops to jump through. I think we really need to get a fresh, clean data.prep output built without this frustrating, known problem so we can test without so many guesses.

MonikaFu commented 5 months ago

@cjyetman makes sense. Then I will leave it open and mark as 'blocked' on ADO until we get a new version of data.prep.

jdhoffa commented 5 months ago

Depends on: https://dev.azure.com/RMI-PACTA/2DegreesInvesting/_workitems/edit/10505

cjyetman commented 5 months ago

For what it's worth, I jumped through all of the hoops (I think), and with @MonikaFu's additions, this appears to work as expected! 👍🏻

MonikaFu commented 5 months ago

@cjyetman does it mean that you tested with the right data? And it works? Can we merge?

cjyetman commented 5 months ago

@cjyetman does it mean that you tested with the right data? And it works? Can we merge?

Yeah, I manufactured an inputs dataset that has the expected scenario name. There were still a few problems (like the JS errors mentioned above), but I believe those are separate issues. I think this PR is safe, but I leave it up to you to decide if/when to merge.