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

Enhance calibration report #1671

Merged
merged 7 commits into from
May 15, 2024
Merged

Enhance calibration report #1671

merged 7 commits into from
May 15, 2024

Conversation

fbenke-pik
Copy link
Contributor

@fbenke-pik fbenke-pik commented May 8, 2024

Purpose of this PR

Enhance output script reportCEScalib to include additional plot formats (credit goes to @0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q). Moved code to a Rmarkdown file.

An example of the new pdf report can be found here: /p/tmp/ricardar/remind/remind-develop/remind/output/SSP2EU-NPi-calibrate_2024-03-21_12.23.31/CES_calibration_report_SSP2EU-NPi-calibrate.pdf

Type of change

  • Refactoring
  • New feature
  • Minor change (default scenarios show only small differences)

Checklist:

  • My code follows the coding etiquette
  • I performed a self-review of my own code
  • I explained my changes within the PR, particularly in hard-to-understand areas
  • All automated model tests pass (FAIL 0 in the output of make test)
  • The changelog CHANGELOG.md has been updated correctly

@fbenke-pik fbenke-pik force-pushed the calib branch 2 times, most recently from 4694689 to 4c528d5 Compare May 8, 2024 14:28
Copy link
Member

Choose a reason for hiding this comment

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

One point: My original script does some shuffling to get the production factors in a sensible order in the plots.

pf_order <- read_lines(file = CES_cal_file, n_max = 1)
if (grepl('^# pf order:', pf_order)) {
    pf_order <- pf_order %>% 
        sub('^# pf order: ', '', .) %>% 
        strsplit(', ') %>% 
        unlist()
} else {
    pf_order <- d %>% 
        pull('pf') %>% 
        unique()
}

d <- d %>%
    order.levels(pf = pf_order)

The result is that production factors within the same CES nest are grouped together in the plot
image
instead of alphabetical order, which is rather meaningless
image

@ricardarosemann
Copy link
Contributor

Thank you for this, very helpful. I still have some comments:

  • Currently, the pdf contains an empty section "Line Plots" in the beginning, this should be deleted.
  • The regions appear in a different order for the two sections "Line Plots" and "Heat map", which is a bit confusing. The "Line Plots" section appears in the order that Remind prescribes (when specifying the region set), while the "Heat map" section is alphabetical. I don't have any hard feelings which of these orders is better, but it should be the same throughout the document.
  • I'm still not fully happy with the coloring of the line plots. I find iterations 1 and 10 a bit easier to tell apart now, but this could still improve. Moreover, both of these iterations can now be confused with the target. It might even make sense to give the last iteration (usually iteration 10) a specific color (rather than relying on a ggplot-color spectrum), as this usually is what will be used in the end. Maybe you can recreate the reporting pdf with my results in /p/tmp/ricardar/remind/adjH2-for-merge/remind/output/incorrectCalibration/SSP2EU-EU21-NPi-calibrate_2024-03-13_17.39.20? That's a calibration run with really ugly results, so it might be an interesting test case.

@fbenke-pik
Copy link
Contributor Author

One point: My original script does some shuffling to get the production factors in a sensible order in the plots.

pf_order <- read_lines(file = CES_cal_file, n_max = 1)
if (grepl('^# pf order:', pf_order)) {
    pf_order <- pf_order %>% 
        sub('^# pf order: ', '', .) %>% 
        strsplit(', ') %>% 
        unlist()
} else {
    pf_order <- d %>% 
        pull('pf') %>% 
        unique()
}

d <- d %>%
    order.levels(pf = pf_order)

The result is that production factors within the same CES nest are grouped together in the plot image instead of alphabetical order, which is rather meaningless image

This should be fixed

@fbenke-pik
Copy link
Contributor Author

Currently, the pdf contains an empty section "Line Plots" in the beginning, this should be deleted.

This is not an empty section, it is the heading for all the plots that follow. I have to introduce a page break, because the line plots in these section need all the space of the page, otherwise the formatting goes wrong and swallows a plot. So this is a know inconvenience that won't go away. The heading is there to allow jumping in the table of contents.

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented May 13, 2024

Currently, the pdf contains an empty section "Line Plots" in the beginning, this should be deleted.

This is not an empty section, it is the heading for all the plots that follow. I have to introduce a page break, because the line plots in these section need all the space of the page, otherwise the formatting goes wrong and swallows a plot. So this is a know inconvenience that won't go away. The heading is there to allow jumping in the table of contents.

image

You have a 2nd-level heading (## Line Plots), then load data (which does not show in the PDF) then a 3ed-level heading (### Quantity Outliers). No Point in that first heading. (And then a 3rd-level heading "Line Plots" further down, but that is not the one Ricarda is talking about.)

@fbenke-pik
Copy link
Contributor Author

You have a 2nd-level heading (## Line Plots), then load data (which does not show in the PDF) then a 3ed-level heading (### Quantity Outliers). No Point in that first heading. (And then a 3rd-level heading "Line Plots" further down, but that is not the one Ricarda is talking about.)

I see, I adjusted the headings again.

@fbenke-pik
Copy link
Contributor Author

The regions appear in a different order for the two sections "Line Plots" and "Heat map", which is a bit confusing. The "Line Plots" section appears in the order that Remind prescribes (when specifying the region set), while the "Heat map" section is alphabetical. I don't have any hard feelings which of these orders is better, but it should be the same throughout the document.

I adjusted the regions sorting to alphabetical order in the first set of plots as well, so they are unified now.

@fbenke-pik
Copy link
Contributor Author

I'm still not fully happy with the coloring of the line plots. I find iterations 1 and 10 a bit easier to tell apart now, but this could still improve. Moreover, both of these iterations can now be confused with the target. It might even make sense to give the last iteration (usually iteration 10) a specific color (rather than relying on a ggplot-color spectrum), as this usually is what will be used in the end. Maybe you can recreate the reporting pdf with my results in /p/tmp/ricardar/remind/adjH2-for-merge/remind/output/incorrectCalibration/SSP2EU-EU21-NPi-calibrate_2024-03-13_17.39.20? That's a calibration run with really ugly results, so it might be an interesting test case.

The report can be found here: /p/tmp/ricardar/remind/adjH2-for-merge/remind/output/incorrectCalibration/SSP2EU-EU21-NPi-calibrate_2024-03-13_17.39.20/CES_calibration_report_SSP2EU-EU21-NPi-calibrate.pdf

Would any of these palettes fit better? (I picked one of the Diverging sections, "Purple-Green") https://colorspace.r-forge.r-project.org/reference/hcl_palettes.html

But yeah, happy to implement any specific instruction you find useful (I am lacking the user perspective here)

@ricardarosemann
Copy link
Contributor

Would any of these palettes fit better? (I picked one of the Diverging sections, "Purple-Green") https://colorspace.r-forge.r-project.org/reference/hcl_palettes.html

I had a look and in terms of contrast I think I like the green-orange one best. These are the palettes with 10 different colors for reference:
diverging_color_palettes

@fbenke-pik
Copy link
Contributor Author

How about this?
image

@ricardarosemann
Copy link
Contributor

ricardarosemann commented May 15, 2024

How about this?

I really like this, I think it is now very clearly visible which one is the final iteration and that (in this case) it closely matches the target.

@fbenke-pik fbenke-pik merged commit 7715d90 into remindmodel:develop May 15, 2024
2 checks passed
@fbenke-pik fbenke-pik deleted the calib branch May 15, 2024 10:30
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.

3 participants