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 the MedlynConductance slope parameter to support SpaceVaryingInput #759

Merged
merged 3 commits into from
Sep 10, 2024

Conversation

imreddyTeja
Copy link
Contributor

@imreddyTeja imreddyTeja commented Sep 9, 2024

Purpose

Extend MedlynConductanceParameters to support a slope parameter that is
<: Union{FT, ClimaCore.Fields.Field}
Closes #756 -- this will automatically close issue 756 on PR merge

To-do

  • The clm_data file for vegetation properties and the script that generates it should be changed to reflect
    the actual units.

Content

  • MedlynConductanceParameters struct change to support g1 as Field
  • Constructor for MedlynConductanceParameters change to reflect struct changes
  • Add for loop for tests that check g1 as Float and Field
  • unrelated: change way the vcmix kwarg is passed to the FarquharParameters constructor to unify code style
  • Change g1 in land.jl to use the map in clm_data (see map below)

Previous map image (has mask over ocean and no unit conversion)

med_slope

Re-made map with no mask and converted to sqrt(Pa)

med_slope


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

@imreddyTeja imreddyTeja marked this pull request as ready for review September 9, 2024 21:43
Copy link
Member

@Sbozzolo Sbozzolo left a comment

Choose a reason for hiding this comment

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

Thank you! Please squash commits that need to be squashed before merging

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.

Thank you! This looks great

@kmdeck
Copy link
Member

kmdeck commented Sep 10, 2024

Would it be possible to update the map of g1 here to plot in units of sqrt(Pa)? Why is value over the ocean nonzero/non NaN?

@imreddyTeja
Copy link
Contributor Author

Would it be possible to update the map of g1 here to plot in units of sqrt(Pa)? Why is value over the ocean nonzero/non NaN?

I added an re-made map under the original in the pull request. The original map in the PR had an ocean mask applied to it. vegetation_properties_map.nc lists the_FillValue as NaN, but there are no NaNs in the data. It also seems like there are no zero values in the data. I believe the value over the ocean is 284.60498 $\sqrt{Pa}$ because the median and maximum of the data is also 284.60498 $\sqrt{Pa}$

@kmdeck
Copy link
Member

kmdeck commented Sep 10, 2024

Would it be possible to update the map of g1 here to plot in units of sqrt(Pa)? Why is value over the ocean nonzero/non NaN?

I added an re-made map under the original in the pull request. The original map in the PR had an ocean mask applied to it. vegetation_properties_map.nc lists the_FillValue as NaN, but there are no NaNs in the data. It also seems like there are no zero values in the data. I believe the value over the ocean is 284.60498 P a because the median and maximum of the data is also 284.60498 P a

@braghiere It is not a big deal, but does it make sense to you why the ocean has a nonzero/nonNan value for medlyn slope?

@imreddyTeja Thanks for remaking the plot! I think we can merge.

@braghiere
Copy link
Member

@kmdeck It was just the way it was constructed. Bare soil and water were assigned values in the map but those should be ignored.

@imreddyTeja The values look wrong now. Medlyn g1 is in sqrt(kPa) which can be anywhere from ~1.5 to ~6. The values were "right" but unit names were wrong.

Screenshot 2024-09-10 at 10 23 35 AM

Extend MedlynConductanceParameters to support
g1 (medlyn slope) as a field. Only the struct and
constructor needed to be changed. Tests check behavior
for g1 as Float and Field
@braghiere braghiere requested a review from kmdeck September 10, 2024 17:29
@imreddyTeja imreddyTeja force-pushed the tr/spatially-varying-medlynslope branch from 372dff9 to c018155 Compare September 10, 2024 17:29
@imreddyTeja
Copy link
Contributor Author

Could I have performed the unit conversion incorrectly? I converted to sqrt(Pa) by multiplying by 10^(3/2). This is what the map looks like with no unit conversion or mask:
med_slope

The data without unit conversion ranges from 1.62 to 9.0

@braghiere
Copy link
Member

Could I have performed the unit conversion incorrectly? I converted to sqrt(Pa) by multiplying by 10^(3/2). This is what the map looks like with no unit conversion or mask: med_slope

The data without unit conversion ranges from 1.62 to 9.0

Got it! I see you converted from sqrt(kPa) ) to sqrt(Pa) to be in the same units as VPD. That makes sense! Thanks.

@imreddyTeja imreddyTeja merged commit 1db54a8 into main Sep 10, 2024
11 checks passed
@imreddyTeja imreddyTeja deleted the tr/spatially-varying-medlynslope branch September 10, 2024 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend the MedlynConductance slope parameter to support SpaceVaryingInput
4 participants