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

BUG fixes: sediment alkalinity and sediment C14 #194

Merged

Conversation

jmaerz
Copy link
Collaborator

@jmaerz jmaerz commented Sep 15, 2022

Dear @TomasTorsvik and @JorgSchwinger , I herewith open the pull request linked to issue #193. In addition to the described alkalinity issue, it seemed to me that there was an additional bugfix for C14 needed - please re-check carefully. This branch hasn't been checked on betzy yet, but compiled successfully locally. I left the rest of the isotopes untouched (didn't introduce sulf13 and sulf14, respectively, since it wasn't needed from a bug fixing perspective, while it breakes the consistency between anaerob and e.g. anaerob13). If you wish to introduce sul13,14 for more consistency, let me know. closes #193

@jmaerz jmaerz added bug Something isn't working iHAMOCC Issue mainly concerns the iHAMOCC code base labels Sep 15, 2022
@jmaerz jmaerz self-assigned this Sep 15, 2022
@TomasTorsvik TomasTorsvik linked an issue Sep 15, 2022 that may be closed by this pull request
Copy link
Contributor

@JorgSchwinger JorgSchwinger left a comment

Choose a reason for hiding this comment

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

This looks good to me (I trust that this is now consistent with the water column code - I haven't checked this myself). Regarding the isotope part: it would be good to aim for a version of the code that runs with C-isotopes in the sediment enabled (wasn't done so far). Therefore I would vote for including sulf13/14 in this fix.

@jmaerz
Copy link
Collaborator Author

jmaerz commented Sep 16, 2022

Hi @JorgSchwinger , the sulf13,14 is not a requirement to enable running the C-isotopes in sediment - it would only increase readability (the reflection of sulf by sulf13,14 - so introducing it would/should not change the result for the isotopes) - I wasn't introducing it for sake of less memory usage thus far, but I also would be in favor to introduce it. I'll introduce it early next week.

@JorgSchwinger
Copy link
Contributor

Ok, I see. For the C-isotope code, the philosophy was to reflect every relevant line in the C12 code by one corresponding line in the C13/14 code. So I agree, I would also be in favor of introducing sulf13/14 even if it's not strictly necessary.

@jmaerz
Copy link
Collaborator Author

jmaerz commented Sep 16, 2022

Ok, thanks for that feedback and valuable information! I'll take care of it beginning of next week.

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.

Looks fine, no complaints from my side

@jmaerz jmaerz merged commit 1470d27 into NorESMhub:feature-hamocc_beyond-CMIP6 Sep 20, 2022
jmaerz added a commit to jmaerz/BLOM that referenced this pull request Aug 9, 2023
Bug fixes for denitrification stoichiometry and C-isotopes in the sediment
@jmaerz jmaerz deleted the fix_sed_alkalinity branch November 23, 2023 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working iHAMOCC Issue mainly concerns the iHAMOCC code base
Projects
None yet
Development

Successfully merging this pull request may close these issues.

iHAMOCC: Bugfix for sediment alkalinity needed?
3 participants