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.

Sign up for GitHub

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

Jump to bottom

All CO2 fluxes in mol CO2 m-2 s-1 #731

Merged
merged 1 commit into from
Aug 20, 2024
Merged

All CO2 fluxes in mol CO2 m-2 s-1 #731

merged 1 commit into from
Aug 20, 2024

Conversation

AlexisRenchon
Copy link
Member

@AlexisRenchon AlexisRenchon commented Aug 19, 2024 •
edited
Loading

There was some inconsistencies, as well as some bug in the code, because of sometimes expressing CO2 fluxes in gC m-2 s-1, and sometimes in mol CO2 m-2 s-1.
In this PR, we express all CO2 fluxes in mol CO2 m-2 s-1. Note: the parameter "f1" (:mol_CO2_to_kg_C_factor) from AutotrophicRespirationParameters has been removed, and the parameter "f2" (:relative_contribution_factor) renamed to "Rel". This change is also happening in ClimaParams.jl PR #205

@AlexisRenchon AlexisRenchon marked this pull request as draft August 19, 2024 22:27
This was referenced Aug 19, 2024
Merged
Closed
@AlexisRenchon
Copy link
Member Author

@kmdeck
I think this PR is pretty close, but there is a remaining thing:
CO2 concentration is in kg C m^-3.
We use it at the top BC for soilco2.
Now, I express microbial source flux in mol CO2 m^-3 s^-1.
I imagine we should express CO2 concentration at BC in mol CO2 m^3.
Where is the best place to do this? In drivers.jl, or in Biogeochemistry.jl?

kmdeck
kmdeck reviewed Aug 20, 2024
View reviewed changes
ext/CreateParametersExt.jl Outdated Show resolved Hide resolved
kmdeck
kmdeck reviewed Aug 20, 2024
View reviewed changes
Copy link
Member

@kmdeck kmdeck left a comment

Choose a reason for hiding this comment

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

so Y.soilco2.C = units of mol CO2/m^3, microbe source is now in mol CO2/m^3/s, and all respiration rates are in mol CO2/m^2/s?

And then the last ? is regarding how to convert the driver CO2 concentration?

@AlexisRenchon AlexisRenchon marked this pull request as ready for review August 20, 2024 18:16
@AlexisRenchon AlexisRenchon force-pushed the ar/co2_unit_consistent branch 2 times, most recently from e666e1f to 273a4da Compare August 20, 2024 20:43
kmdeck
kmdeck reviewed Aug 20, 2024
View reviewed changes
lib/ClimaLandSimulations/src/Fluxnet/fluxnet_sites/US-MOz/US-MOz_parameters.jl Outdated
@@ -60,10 +60,9 @@ function auto_resp_ozark(;
σl = FT(0.05),
μr = FT(1.0),
μs = FT(0.1),
f1 = FT(0.012),
f2 = FT(0.25),
f = FT(0.012),
Copy link
Member

Choose a reason for hiding this comment

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

should be Rel?

Copy link
Member

Choose a reason for hiding this comment

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

(f no longer in struct?)

kmdeck
kmdeck reviewed Aug 20, 2024
View reviewed changes
src/diagnostics/land_compute_methods.jl
land_model::SoilCanopyModel{FT},
) where {FT}
if isnothing(out)
return p.soilco2.top_bc .* FT(83.26)
Copy link
Member

@kmdeck kmdeck Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe we could note that:
"To convert from kg C to mol CO2, we need to multiply by: [3.664 kg CO2/ kg C] x [10^3 g CO2/ kg CO2] x [1 mol CO2/44.009 g CO2] = 83.26 mol CO2/kg C"

Copy link
Member

Choose a reason for hiding this comment

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

Technically 83.26 is a parameter in ClimaParams. Would it be possible to fetch its value from land_model?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder if I should remove it from ClimaParams, as it is just a conversion?

Copy link
Member Author

Choose a reason for hiding this comment

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

that I added but actually we are not using it (except if we do use it here)

Copy link
Member

Choose a reason for hiding this comment

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

To make it makes sense that it should not be a parameter (but there are other constants of nature/conversions in there). I'd remove it.

kmdeck
kmdeck approved these changes Aug 20, 2024
View reviewed changes
Copy link
Member

@kmdeck kmdeck left a comment

Choose a reason for hiding this comment

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

thanks! looks good.

@AlexisRenchon @kmdeck @Sbozzolo
All CO2 fluxes in mol CO2 m-2 s-1
c6dea22 
There was some inconsistencies, as well as some bug in the code,
because of sometimes expressing CO2 fluxes in gC m-2 s-1,
and sometimes in mol CO2 m-2 s-1.
In this PR, we express all CO2 fluxes in mol CO2 m-2 s-1.
Note: the parameter "f1" (:mol_CO2_to_kg_C_factor) from
AutotrophicRespirationParameters has been removed,
and the parameter "f2" (:relative_contribution_factor)
renamed to "Rel". This change is also happening in ClimaParams.jl.

Co-authored-by: AlexisRenchon <[email protected]>
Co-authored-by: kmdeck <[email protected]>
Co-authored-by: SBozzolo <[email protected]>
@AlexisRenchon AlexisRenchon merged commit 0decb8b into main Aug 20, 2024
9 checks passed
@AlexisRenchon AlexisRenchon deleted the ar/co2_unit_consistent branch October 21, 2024 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kmdeck kmdeck kmdeck approved these changes

@Sbozzolo Sbozzolo Awaiting requested review from Sbozzolo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AlexisRenchon @Sbozzolo @kmdeck