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

Refactors gas exchange carbon chemistry (+ adds some complexity) #186

Merged
merged 120 commits into from
Aug 15, 2024

Conversation

jagoosw
Copy link
Collaborator

@jagoosw jagoosw commented Jul 15, 2024

This PR refactors the carbon chemistry calculations improving the clarity and supersedes #179.

It also moves both the carbon chemistry and gas exchange functions to Models since they are models, and separates them.

This PR also implements the full carbon chemistry standards of the best practice (https://www.ncei.noaa.gov/access/ocean-carbon-acidification-data-system/oceans/Handbook_2007/Guide_all_in_one.pdf) so it can depend on Si and PO4, and approximated fluoride and sulfate. It also introduces proper (TEOS10 and polynomial approximations) density calculations.

Also, all of the equilibrium and pressure correction constants are also tested now.

I also introduce a function calcite_saturation that can compute calcite_concentration and saturation for use in other models.

This should also be faster since it removed a branching operation.

Finally, I changed the structure of GasExchange to make it more flexible (and correct, there was a bug in the computation of x(CO2) from fCO2).

Also, closes #197, #200

@jagoosw jagoosw changed the title Refactors carbonate chemistry + adds some complexity Refactors carbon chemistry + adds some complexity Jul 15, 2024
@jagoosw
Copy link
Collaborator Author

jagoosw commented Jul 15, 2024

@bethcandish you could do some testing with the "best practice" parameters with this branch and see if it makes any difference, and if so change them here.

@@ -151,12 +151,12 @@ resulting in the equilibrium constant:
K_B = \frac{\ce{[B(OH)^-_4][H^+]}}{\ce{[B(OH)_3]}}.
```

Finally, taking the DIC and Alkalinity in micro equivalents (i.e. scaled by ``10^{-3}/\rho_o`` from mmol C/m``^3``) denoted by ``\bar{DIC}`` and ``\bar{Alk}``, the carbonate alkalinity is given by ``Alk_C = \bar{Alk} + [B(OH)^-_4] + [OH^-]``, and we define the boron content, ``B``, to be ``R_{B:S}S`` mol/kg.
Finally, taking the DIC and Alkalinity in mol / kg and equivilants / kg (i.e. scaled by ``10^{-3}/\rho_o``) denoted by ``\bar{DIC}`` and ``\bar{Alk}``, the carbonate alkalinity is given by ``Alk_C = \bar{Alk} + [B(OH)^-_4] + [OH^-]``, and we define the boron content, ``B``, to be ``R_{B:S}S`` mol/kg.
Copy link
Collaborator

Choose a reason for hiding this comment

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

spelling should be equivalents, I think

Copy link
Collaborator

@johnryantaylor johnryantaylor left a comment

Choose a reason for hiding this comment

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

The edits look good to me. There are some comments in the carbon chemistry function that can be cleaned up. It was a great idea to put the carbon ehcmistry as a separate model type - this will be very useful!

# Centigrade to kelvin
T += 273.15

# mili-equivilants / m³ to equivilants / kg
Copy link
Collaborator

Choose a reason for hiding this comment

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

spelling should be "equivalents" throughout

@jagoosw
Copy link
Collaborator Author

jagoosw commented Jul 19, 2024

@johnryantaylor do you think it would make sense to move carbon chemistry to be a property of Biogeochemistry?

Previously we had only used it in the CO2 flux but PISCES needs the calcite saturation for various things, and it makes sense to me that we separate it so it can be used for any model.

It wouldn't cause many changes and will just be there to be called, e.g. by the boundary condition.

@jagoosw
Copy link
Collaborator Author

jagoosw commented Jul 19, 2024

Also, I will update the docs page at some point but the code now includes the pressure correction for the rate constants. I have added some tests which ensure we have the constants right (when using the "best practice" parameters) like how Kgen does, so I think the only difference between the functionality here and cbsyst on the carbon front is the Mg and Ca bits.

@jagoosw
Copy link
Collaborator Author

jagoosw commented Jul 19, 2024

Final comment for now, I'm pretty sure that this PR isn't breaking since OCMPI_default wasn't part of the public API, but it's not clear to me if it counts as breaking because lots of things have moved around. I don't really want to make a breaking release because it was a lot of effort to bump the compact of all my packages that rely on it like the giant kelp stuff.

@jagoosw
Copy link
Collaborator Author

jagoosw commented Aug 6, 2024

TODO: check model_implemntation page when rendered

update: it was broken

@jagoosw jagoosw requested a review from chloeyhuang August 7, 2024 14:46
silicate = data[n, Si_name] * density * 1e-3
phosphate = data[n, PO_name] * density * 1e-3

silicate = ifelse(silicate == -9999, 0, silicate)
Copy link
Collaborator

@chloeyhuang chloeyhuang Aug 13, 2024

Choose a reason for hiding this comment

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

the ifelse statement should probably go before the units of silicate and phosphate are converted? Same with P

@jagoosw jagoosw merged commit d4802b1 into main Aug 15, 2024
5 checks passed
@jagoosw jagoosw deleted the jsw/refactor-carbonate-chemistry branch August 15, 2024 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request science
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Non-zero CO2 surface flux for an ocean in equilibrium (with non-zero average wind speed)
4 participants