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

Extend Vcmax to support SpaceVaryingInput #744

Merged
merged 2 commits into from
Sep 5, 2024

Conversation

imreddyTeja
Copy link
Contributor

@imreddyTeja imreddyTeja commented Aug 30, 2024

Purpose

FarquharParameters now also supports Vcmax25 of type ClimaCore.Fields.Field. In relevant tests, Vcmax25 is changed from a float to a field that contains the previous Vcmax25 value everywhere. Both longrun and benchmark Land.jl now use the Vcmax map from clm_data.
Closes #723

PR also contains whitespace removal.

To-do

  • Add VCmax map to ClimaArtifacts

Content

  • FarquharParameters takes a Vcmax25 of type F <: Union{<: AbstractFloat, ClimaCore.Field}
  • Land.jl benchmark and long run uses the vcmax map from clm_data. Map can be seen below.
    vc
  • Relevant tests now use field of constant values for Vcmax25.

Example output of experiments/long_runs/land.jl

Previously:

Day6Prev

After changing Vcmax25 to the clm_data map:

gpp_518400 0


  • I have read and checked the items on the review checklist.

@imreddyTeja imreddyTeja force-pushed the tr/extend-vcmax-to-field branch 2 times, most recently from 1b89316 to 7165e69 Compare August 30, 2024 19:21
@imreddyTeja imreddyTeja requested a review from Sbozzolo August 30, 2024 20:19
@imreddyTeja imreddyTeja force-pushed the tr/extend-vcmax-to-field branch from 7165e69 to 2755c4f Compare August 30, 2024 20:24
@imreddyTeja imreddyTeja marked this pull request as ready for review August 30, 2024 20:27
@imreddyTeja imreddyTeja marked this pull request as draft August 30, 2024 20:51
@imreddyTeja imreddyTeja force-pushed the tr/extend-vcmax-to-field branch from 2755c4f to 1d4db35 Compare August 30, 2024 21:13
@imreddyTeja imreddyTeja marked this pull request as ready for review August 30, 2024 21:19
@imreddyTeja imreddyTeja force-pushed the tr/extend-vcmax-to-field branch from 1d4db35 to b76c664 Compare August 30, 2024 23:15
@imreddyTeja imreddyTeja requested review from kmdeck and removed request for kmdeck September 4, 2024 15:49
FarquharParameters now also supports Vcmax25 of type
ClimaCore.Fields.Field. In relevant tests, Vcmax25 is changed from
a float to a field that contains the previous Vcmax25 value everywhere.
Both longrun and benchmark Land.jl now use the Vcmax map from
clm_data.
@imreddyTeja imreddyTeja force-pushed the tr/extend-vcmax-to-field branch from b76c664 to 5a015dc Compare September 4, 2024 16:21
@@ -166,7 +166,9 @@ function FarquharParameters(
parameters = CP.get_parameter_values(toml_dict, name_map, "Land")
FT = CP.float_type(toml_dict)
MECH = typeof(mechanism)
return FarquharParameters{FT, MECH}(;
Vcmax25 = FT.(Vcmax25)
Copy link
Member

Choose a reason for hiding this comment

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

do we need this FT? If Vcmax25 is a field, it should already be made on the space of the simulation and be the right type. If it is a scalar, this definition in the FarquharParameter struct VC <: Union{FT, ClimaCore.Fields.Field} should ensure it is the same type as the other scalars.

Copy link
Contributor Author

@imreddyTeja imreddyTeja Sep 5, 2024

Choose a reason for hiding this comment

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

I think Julia did not want to perform conversion from scalars to a union of a scalar and a non-scalar. If I remove line 169 the tests fail with
TypeError: in FarquharParameters, in VC, expected VC<:Union{Float32, ClimaCore.Fields.Field}, got Type{Float64}. Additionally, if I try converting from a scalar to Union{FT, ClimaCore.Fields.Field} I get
MethodError: Cannot convert an object of type Float64 to an object of type Union{Float32, ClimaCore.Fields.Field}

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.

This looks good to me, thank you!

We must already broadcast over functions with VCmax25 in Canopy.jl, since that didnt need to change?

@Sbozzolo
Copy link
Member

Sbozzolo commented Sep 4, 2024

Make sure to add an entry to the NEWS file too. When you do so, it is nice if you can showcase the new feature you have added (see, e.g., how we do it in ClimaAnalysis).

@imreddyTeja imreddyTeja merged commit 0fe7a46 into main Sep 5, 2024
12 checks passed
@imreddyTeja imreddyTeja deleted the tr/extend-vcmax-to-field branch September 5, 2024 00:11
@AlexisRenchon
Copy link
Member

beautiful! thanks @imreddyTeja

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend Vcmax to support SpaceVaryingInput
4 participants