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

rename and improve description of weathering rates #1929

Merged
merged 5 commits into from
Feb 3, 2025

Conversation

tabeado
Copy link
Contributor

@tabeado tabeado commented Dec 12, 2024

Purpose of this PR

p33_co2_rem_rate and s33_co2_rem_rate currently have the same description, albeit meaning different things.

To make it more accessible and have different names, I suggest to rename the latter to s33_co2_rem_fraction, as it actually provides the (theoretical) fraction weathering each year. Alternatively, we could call it p33_co2_rem_exponent?

The PR also changes the description in the declarations.

Happy to further improve the description or names based on your suggestions.

If you agree to the renaming, I will also need to adjust it in remind2.

Type of change

  • Bug fix

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
  • I checked that the in-code documentation is up-to-date
  • I adjusted the reporting in remind2 where it was needed
  • I adjusted forbiddenColumnNames in readCheckScenarioConfig.R in case the PR leads to deprecated switches
  • All automated model tests pass (FAIL 0 in the output of make test)
  • The changelog CHANGELOG.md has been updated correctly

@strefler @amerfort

for reference @VerenaHof @robertsalzwedel

@tabeado tabeado requested review from strefler and amerfort December 12, 2024 15:33
@tabeado
Copy link
Contributor Author

tabeado commented Dec 19, 2024

Updated suggestion to be approved by @strefler:

  • We renamed s33_co2_rem_rate and p33_co2_rem_rate to s33_rock_weath_rate_ambientT and p33_rock_weath_rate:
    - The scalar provides the fraction of stone weathering each year at ambient temperature. This is now reflected in the name.
    - The parameter provides the fraction of stone weathering each year for the two climate grades.
  • We simplify the equations q33_EW_onfield_tot and q33_EW_emi:
    - The logarithmus in the old p33_co2_rem_rate and the exponential functions in these equations cancel out. We suggest to avoid the exponential formulation. Or was there a computational GAMS reason to write it this way?
    - The change means that the content of p33_rock_weath_rate has a direct simple interpretation (changed from the exponent of the exponential function to simply the rock fraction weathering per year)

@tabeado
Copy link
Contributor Author

tabeado commented Dec 19, 2024

Test run with the new formulation: /p/tmp/tabeado/remind_clean_new/remind/output/SSP2-PkBudg650-EW_2024-12-18_19.04.40

image

@strefler
Copy link
Contributor

Updated suggestion to be approved by @strefler:

* We renamed `s33_co2_rem_rate `and  `p33_co2_rem_rate `to  `s33_rock_weath_rate_ambientT` and  `p33_rock_weath_rate`:
  - The scalar provides the fraction of stone weathering each year at ambient temperature. This is now reflected in the name.
  - The parameter provides the fraction of stone weathering each year for the two climate grades.

* We simplify the equations `q33_EW_onfield_tot `and `q33_EW_emi`:
  - The logarithmus in the old `p33_co2_rem_rate` and the exponential functions in these equations cancel out. We suggest to avoid the exponential formulation. Or was there a computational GAMS reason to write it this way?
  - The change means that the content of `p33_rock_weath_rate ` has a direct simple interpretation (changed from the exponent of the exponential function to simply the rock fraction weathering per year)

Thanks! I like the renaming. Regarding the simplification, I'm not sure whether it is easier for GAMS to deal with an expression like c * exp(x), or x**c. Both of them involve power functions, and my gut feeling is, that exponential might be easier, but I'm not sure. However, in the last change of the equations file, there is one exponential less, so maybe that helps. Might make sense to involve RSE on that question.

The test runs show quite a bit of change, I would have expected pretty much the same outcome. Do we know where the deviation comes from?

@tabeado
Copy link
Contributor Author

tabeado commented Jan 29, 2025

@strefler why do you think it looks different to before?
I can make another test run that compares the suggested change to the status quo

@tabeado
Copy link
Contributor Author

tabeado commented Jan 29, 2025

@strefler we consulted @LaviniaBaumstark who shared your concern.

The compromise is to move the log calculation to the equation. This keeps the accessible interpretation of p33_rock_weath_rate and makes also q33_EW_emi simpler to read. The calculation of v33_EW_onfield_tot may look a bit more complicated at first, but I added an explanation in the description which should support understanding. As all exp & log are at the same place now, it also doesn't require to look in 2 documents to understand the equation.

modules/33_CDR/portfolio/datainput.gms Outdated Show resolved Hide resolved
@tabeado
Copy link
Contributor Author

tabeado commented Jan 30, 2025

I made a final check comparing the previous and current formulation, just to be sure. There are very minor deviations that I don't understand. My guess would be it is due to some minor numerical differences in the calculation?
image

It wouldn't stop me from merging, but @amerfort could you make a final check that I didn't make a random mistake in the reformulation?

@tabeado tabeado merged commit a22ec72 into remindmodel:develop Feb 3, 2025
2 checks passed
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