More usage of ClimaParams

[ph-kev]

closes #1374, closes #1270

This PR removes the constructor for parameters that takes in FT and replace them if they did not exist already with constructor using toml_dict. Furthermore, the parameters for the P model is now in the TOML file.

• Check documentation (also see Add constructors API #1367 (comment))

[ph-kev]

I am not sure what parameters to expose to the user.

[kmdeck]

I think this is OK as is (nothing exposed).

[kmdeck]

(but then removing the kwargs..., right? I believe we had discussed that being too ambigious to users. )

[ph-kev]

I am not sure what parameters to expose to the user.

[kmdeck]

this should not be in the toml dict. It can be set via kwarg (but not have a default - in which case maybe making it required is better?), or we can set the default from the domain by reading in the parameter map. (Spatially varying params should not be in the toml dict)

it should have the same type as is_c3, and should be treated the same as is_c3 (see earlier comment) in terms of kwarg vs required arg

[kmdeck]

I think it is OK to only expose is_c3 and Vcmax25

[ph-kev]

I am not sure what parameters should be to exposed to the user

[ph-kev]

I think I covered everything that needed to be done, but there might be some small things that I missed.

[juliasloan25]

A couple comments but this looks good!

As far as which parameters to expose to users, we can keep the same ones we had exposed previously. As far as I understand, this PR doesn't change which parameters are exposed - it changes how the defaults are set, but users can still overwrite the defaults via kwargs.

Also, in experiments/long_runs/low_res_snowy_land_rh.jl, setup_model should take in toml_dict instead of earth_param_set here

[kmdeck]

there is an equivalent method for LandModel (takes component types and args) - should that be updated too?

[ph-kev]

There is

function LandModel{FT}(
    forcing,
    LAI,
    toml_dict::CP.AbstractTOMLDict,
    domain::Union{
        ClimaLand.Domains.Column,
        ClimaLand.Domains.SphericalShell,
        ClimaLand.Domains.HybridBox,
    },
    Δt;

with some more keyword arguments.

[juliasloan25]

I believe @kmdeck is talking about this one. We're planning to remove this type of constructor soon, but while we still have them it would be good for them to remain consistent with each other

[kmdeck]

should this be FarquharParameters(toml_dict, is_c3, Vcmax25; sc, pc) instead? We dont have a default value of Vcmax25.

[kmdeck]

errr sorry, we do (we get it from the domain). So actually, since we do the same for is_c3, should this be
FarquharParameters(domain, toml_dict; is_c3 = , Vcmax25 = , sc = , pc= )?

[kmdeck]

sorry i was a little confused. it looks like we are doing the right thing, the FarquharParameters constructor is just a little confusing (not sure why is_c3 is not kwarg when the others are)

[ph-kev]

I can make is_c3 a keyword argument. I am also not sure why is_c3 is not a keyword argument, but I am guessing it is because it also accepts a ClimaCore.Fields.Field.

[juliasloan25]

I may have gotten confused when refactoring the parameters structs and written the constructor like this. I think is_c3 can be a kwarg for FarquharParameters like it is for PModel further down in the file -

is_c3 = clm_photosynthesis_parameters(domain.space.surface).is_c3,

[kmdeck]

hi Kevin! Thanks for making these changes - it looks like a lot of work.

It all looks good, I left a few comments.

I have a couple questions
What is the general philosophy behind parameter structs, and can we unify this across components?

• Do we basically support two methods for Parameter() structs? one taking the toml (or the toml and domain, with a subset of parameters allowing explicit kwarg overrides), and one taking all parameters by kwarg? I saw this for snow. or do different parameter structs allow different things? It seems like a future goal might be to unify these
• Are required arguments (not in the toml) passed in as kwarg or regular arg? for photosynthesis, we handled is_c3 and vcmax25 differently
• Are the only required arguments or unset kwargs the spatially varying parameters?

Other questions:

• Are the model constructors also using the toml dict uniformly now?
• A lot of our experiment scripts still make the earth_param_set from the toml_dict. Is this required? or should the earth_param_set be constructed internally?

(We dont have to answer these questions to merge this)