move compareScenarios core framework to piamPlotComparison

[fbenke-pik]

This PR moves the generic logic to create a compareScenarios pdf to the library piamPlotComparison.

The following files are no longer needed:

• R/compareScenarios2.R (moved to piamPlotComparison)
• inst/markdown/compareScenarios2/cs2_main.Rmd (moved to piamPlotComparison, remind2 specific preprocesing code was moved to inst/compareScenarios/preprocessing.R)
• inst/markdown/compareScenarios2/cs2_latex_template.tex and inst/markdown/compareScenarios2/cs_pdf_header_include.tex (moved to piamPlotComparison)

The section markdowns used to generate the current pdf are remind2 specific and remain in this repo. To work with the new framework, they were moved to the folder /inst/compareScenarios/ and renamed to cs_NN_TEXT.Rmd (e.g. inst/markdown/compareScenarios2/cs2_01_summary.Rmd -> inst/compareScenarios/cs_01_summary.Rmd).

General-purpose files moved to piamPlotComparison

• R/calcTimeSeriesStats.R
• R/showStatsTable.R
• R/getCs2Profiles.R

Related PR in REMIND: remindmodel/remind#1661

[orichters]

again, I think we should keep remind2::compareScenarios2() as long as remindmodel master calls it. But maybe we should decide more generally if we strive for compatibility of the latest remindmodel release with the most recent remind2 version. Given my experience in ENGAGE, ELEVATE and NGFS, I would strongly opt for making sure that works, because most projects seems to start from the release and then add some changes to it that require adding stuff to remind2. The most convenient way to avoid divergence of project work and model development is to just push that to the remind2 master and work with that. But if compareScenarios2 is broken then, this workflow impossible – which I think creates more problems and unnecessary double-work compared to making sure we allow for backwards compatibility. Maybe we should discuss that more generally within the remind team.

[fbenke-pik]

Personally, I am not a fan of coding for hypothetical causes. I have a hard time wrapping my head around a specific use case where you cannot simply merge the latest REMIND and remind2 into your project branches (unless you never want to merge your work back into the master branches).

On the other hand, I am very familiar with the technological debt created by writing and maintaining code for all eventualities. So I personally would recommend to merge this and offer help to any project that actually ends up struggling with this on a case by case basis.

As I want this to be merged, I am bringing back compareScenarios2 for now, but will raise this topic with RSE.

[orichters]

It is not hypothetical. It is the usual workflow in every single project I'm involved in (NGFS, ELEVATE, COMMITTED) and I guess everywhere when you want to use a validated REMIND version. The current develop is neither validated nor stable and I would not recommend to use it for project work. But yes, please discuss that within RSE and I think we should discuss that in the REMIND meeting.

[fbenke-pik]

it is hypothetical for me, because I don't know any specific use case yet.

because most projects seems to start from the release and then add some changes to it that require adding stuff to remind2

What kind of changes would you add to your REMIND project branch that

• require new reporting functionality (or sth. else?) in remind2 develop branch
• yet at the same time do not collide with REMIND develop, which would also have to work with your remind2 changes, but does not have your diverging REMIND code?

Wouldn't it be in fact be better to add these changes to your remind2 project branch, as you cannot be sure that they work well with REMIND develop?

[fbenke-pik]

Btw, in this specific case of compareScenarios, it would be much easier to fix the call of compareScenarios in the REMIND R script (https://github.com/remindmodel/remind/blob/develop/scripts/cs2/run_compareScenarios2.R) of your project branch if your run into compatibility issues than writing a reexport function to remind2.

[fbenke-pik]

it is hypothetical for me, because I don't know any specific use case yet.

because most projects seems to start from the release and then add some changes to it that require adding stuff to remind2

What kind of changes would you add to your REMIND project branch that

* require new reporting functionality (or sth.  else?)  in remind2 develop branch

* yet at the same time do not collide with REMIND develop, which would also have to work with your remind2 changes, but does not have your diverging REMIND code?

Wouldn't it be in fact be better to add these changes to your remind2 project branch, as you cannot be sure that they work well with REMIND develop?

Also, wouldn't you risk running into inconsistencies anyways if you update remind2 to latest develop (assuming it adds new features that need the REMIND develop updates), but keep to the latest REMIND release version + your own changes?

[orichters]

Btw, in this specific case of compareScenarios, it would be much easier to fix the call of compareScenarios in the REMIND R script (https://github.com/remindmodel/remind/blob/develop/scripts/cs2/run_compareScenarios2.R) of your project branch if your run into compatibility issues than writing a reexport function to remind2.

Ok, that is fine as well. But then we should announce that at the REMIND meeting and in mattermost, telling projects to cherry-pick those changes if they work with the newest remind2 version.

What kind of changes would you add to your REMIND project branch that

• require new reporting functionality (or sth. else?) in remind2 develop branch
• yet at the same time do not collide with REMIND develop, which would also have to work with your remind2 changes, but does not have your diverging REMIND code?

Wouldn't it be in fact be better to add these changes to your remind2 project branch, as you cannot be sure that they work well with REMIND develop?

Of course, we merge these things back to remindmodel when we commit to remind2. But that does not imply we take over everything going on in REMIND develop to our project branches.

Example: this commit. We add a new variable that requires this PR in remindmodel. But to avoid that all people using remind2 after our commit on runs that are some days old, we put them behind a conditional to avoid fails for other people.

Also, wouldn't you risk running into inconsistencies anyways if you update remind2 to latest develop (assuming it adds new features that need the REMIND develop updates), but keep to the latest REMIND release version + your own changes?

Yes, maybe. Still, the existing tests in remind2 should make sure the latest master release is still supported (at least does not create errors).

Overall, I'm not sure our workflow is ideal. It is the workflow I got used to because my first work here was to merge hundreds of changes made by Christoph and Jerome back to remind2 and remindmodel develop because the branches had diverged over a year. Therefore, I try to merge back as soon as possible. And if another workflow is preferable, I'm happy to learn from best practices in other projects.

[fbenke-pik]

Turns out the most straightforward way is to bring back compareScenarios2, so I did that for now. REMIND release cs2 scripts should now work with the latest remind2.

[orichters]

Will we keep compareScenarios2 here or should we move to compareScenarios as well in remindmodel?

[fbenke-pik]

I don't mind renaming it in the future. The rationale behind keeping the name compareScenarios2 / cs2 for now is that it could be useful to differentiate between:

• piamPlotComparison::compareScenarios: a generic function independent of the reporting library (remind2, reporttransport, reportbrick, ...)
• compareScenarios2 a function to call the compareScenarios framework in the context of the remind2 reporting library comparing REMIND runs

[orichters]

same here with cs2

[orichters]

But most chunks have names, see https://github.com/pik-piam/remind2/blob/master/inst/markdown/compareScenarios2/cs2_02_macro.Rmd

[fbenke-pik]

I know, this text was written for the vignette by Christof when creating compareScenarios2 and I migrated all the parts that are not outdated to the new vignette. In practice, they did not stick to this convention. I think, it is meant as a warning for users when introducing new chunks so they are aware that this might be a source of compilation errors. We could rephrase it to make that clearer, but I think we want to keep the existing chunk names.