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

Add linear ramping-up scenario for ocean alkalinisation #224

Merged
merged 6 commits into from
Jan 31, 2023

Conversation

TimotheeBrgs
Copy link
Collaborator

@TimotheeBrgs TimotheeBrgs commented Jan 23, 2023

mo_read_oafx.F90 was previously including 2 ocean alkalinisation scenarios with constant alkalinity addition over time. This commit adds another scenario with linear ramping up of alkalinity addition from 0 Pmol alk/yr in year ramp_start to addalk_ramp Pmol alkalinity/yr in year ramp_end. Then the alkalinity addition is kept constant after ramp_end.

@TomasTorsvik TomasTorsvik added the iHAMOCC Issue mainly concerns the iHAMOCC code base label Jan 24, 2023
Copy link
Contributor

@TomasTorsvik TomasTorsvik left a comment

Choose a reason for hiding this comment

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

The code contribution looks fine to me, so I will approve it. The only question I have is whether or not the parameters can be read in a more sensible way.

hamocc/mo_read_oafx.F90 Outdated Show resolved Hide resolved
@TomasTorsvik
Copy link
Contributor

TomasTorsvik commented Jan 24, 2023

@TimotheeBrgs
Hi, I made branch in my personal fork based on this PR with a suggestion for how to read the parameters from a namelist file
https://github.com/TomasTorsvik/BLOM-TTfork/tree/OA_rampscen_read
Would this soltion work for you?

Here is the commit diff:
TomasTorsvik@69f30fe

@jmaerz
Copy link
Collaborator

jmaerz commented Jan 24, 2023

Hi @TimotheeBrgs , I agree with @TomasTorsvik that it would be good to discontinue hard-coding individual cases and make the structure more flexible to user demands (we touched upon it in #187). @TomasTorsvik , thanks for the alternative solution! One could even further simplify the routine get_oafx and only write it in the form of the ramping (the moment that ramp_start == ramp_end, the whole procedure reduces to the formerly implemented case, I believe) - but maybe this reduces a bit the readability (so more meant as an option). To keep the information on the other two use cases of alkalinization, it would be good to automatically adjust the ADDALK_CONST parameter in the buildnml according to the scenario - so everything could be handled via the nml. Further, even though Fortran should do it internally, an explicit casting from int to float would be nice for the oafx calculation (or at least 365. instead of 365).

@TomasTorsvik
Copy link
Contributor

Here is the commit diff: TomasTorsvik@69f30fe

Forgot to re-label the namelist entry ...
TomasTorsvik@9725deb

hamocc/mo_read_oafx.F90 Outdated Show resolved Hide resolved
@TimotheeBrgs
Copy link
Collaborator Author

Hi all, thank you for reviewing this, the suggestions and the proposed commit.

From @TomasTorsvik's commit, I simplified the code by merging the 2 constant scenarios into a single const scenario, knowing that the flux can now be defined externally. I use now 365. instead of 365 to explicitly cast from int to float as suggested. I suggest keeping the distinction between const and ramp scenarios for the readibility. All ocean alkalinization parameters can now be defined in the namelist user_nl_blom. I rearranged the buidnml in order to get all oafx-related parameters in the BGCOAFX section.

There is a mention of addalk_const as a comment on line 97 of @TomasTorsvik's example , this variable does not exist and might have created confusion with the ADDALK_TOT of the original version of the code. I renamed ADDALK_TOT by just ADDALK to improve readability.

hamocc/mo_read_oafx.F90 Outdated Show resolved Hide resolved
Copy link
Contributor

@TomasTorsvik TomasTorsvik left a comment

Choose a reason for hiding this comment

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

I suggest some minor modifications. Accept if you agree, it should be included in the PR automatically.

hamocc/mo_read_oafx.F90 Outdated Show resolved Hide resolved
cime_config/buildnml Outdated Show resolved Hide resolved
cime_config/buildnml Outdated Show resolved Hide resolved
TimotheeBrgs and others added 2 commits January 30, 2023 16:29
Initiate naming convention for shell variables in buildnml such as NAMELIST_VARIABLE

Co-authored-by: Tomas Torsvik <[email protected]>
Copy link
Collaborator

@jmaerz jmaerz left a comment

Choose a reason for hiding this comment

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

Just go ahead from side. Approved.

@TomasTorsvik TomasTorsvik merged commit ef8f025 into NorESMhub:master Jan 31, 2023
@TimotheeBrgs TimotheeBrgs deleted the OA_rampscen branch January 31, 2023 10:24
jmaerz pushed a commit to jmaerz/BLOM that referenced this pull request Aug 9, 2023
Add linear ramping-up scenario for ocean alkalinisation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iHAMOCC Issue mainly concerns the iHAMOCC code base
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants