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

New sub-section and standard names for land surface variables #60

Merged
merged 2 commits into from
Apr 11, 2024

Conversation

SamanthaPullen
Copy link
Contributor

This PR addresses Issue #59

This PR adds some variable names to the CCPP Standard Names for land surface variables, in preparation for Met
Office work on land surface DA with JEDI. I think it makes sense to add a new sub-section for these surface variables. I have added a subsection land_surface, but it could be a wider scope sub-section covering all surface (i.e. land, sea, sea-ice). Please advise :)

I have added the following land surface variable names, in alphabetical order (which would be nice to see elsewhere in the CCPP Standard Names list, for ease of reference):

  1. land_ice_area_fraction
  2. mass_content_of_water_in_top_soil_layer
  3. surface_snow_density
  4. urban_area_fraction
  5. volume_fraction_of_liquid_water_in_soil_at_critical_point
  6. volume_fraction_of_liquid_water_in_soil_at_saturation
  7. volume_fraction_of_liquid_water_in_soil_at_wilting_point

I have used CF Standard Names (cfconventions.org) where possible, and where nothing suitable exists I have based the proposed CCPP name on the CCPP or CF name of a related or similar variable.

Opened as a draft PR for now to allow coordination between Met Office and JCSDA before submitting for review. Comments welcome in the meantime!

@BenjaminRuston
Copy link

thanks @SamanthaPullen ,,,, I'm not part of this repository but @svahl991 wanted to make sure the JCSDA OBS team was also aware.

should tag @ClaraDraper-NOAA as well but may need to send her this link in an email

@mkavulich mkavulich removed the request for review from cacraigucar March 18, 2024 21:14
@SamanthaPullen SamanthaPullen marked this pull request as ready for review March 20, 2024 09:03
@SamanthaPullen
Copy link
Contributor Author

These additions have been discussed with @ClaraDraper-NOAA and she has confirmed that she is happy with them.

@ClaraDraper-NOAA
Copy link

These additions have been discussed with @ClaraDraper-NOAA and she has confirmed that she is happy with them.

My apologies, I responded only to the email. These names are fine by me. Thanks for getting this ready @SamanthaPullen

Copy link
Collaborator

@nusbaume nusbaume left a comment

Choose a reason for hiding this comment

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

Looks good, but I did have one concern that should probably be discussed more widely amongst the group, or at least among folks who know more about land surface modeling than me.

standard_names.xml Show resolved Hide resolved
Copy link
Collaborator

@nusbaume nusbaume left a comment

Choose a reason for hiding this comment

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

My one concern has been resolved. Thanks!

@SamanthaPullen
Copy link
Contributor Author

There is some associated discussion on the naming conventions and precise definitions of variables for area fractions in #57. This could have implications for the variable names land_ice_area_fraction and urban_area_fraction in this PR

@SamanthaPullen SamanthaPullen marked this pull request as draft March 28, 2024 16:43
@SamanthaPullen
Copy link
Contributor Author

I have switched this PR to draft given the related discussion in #57. Will update names for land_ice_area_fraction and urban_area_fraction by appending _of_cell_area after the Easter Holiday and re-open for review

@SamanthaPullen SamanthaPullen marked this pull request as ready for review April 3, 2024 10:32
@SamanthaPullen
Copy link
Contributor Author

@mkavulich @ss421 @svahl991 Following discussion at #57 and for consistency with the naming convention for sea ice fraction variables proposed there, I have modified the following land surface variable names:

  • land_ice_area_fraction
  • urban_area_fraction

modified to:

  • land_ice_area_fraction_of_cell_area
  • urban_area_fraction_of_cell_area

Now ready for review again - @nusbaume are you happy with this update?

Copy link
Collaborator

@MayeulDestouches MayeulDestouches left a comment

Choose a reason for hiding this comment

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

Thanks @SamanthaPullen for this addition. I approve these new names.

@nusbaume
Copy link
Collaborator

nusbaume commented Apr 4, 2024

@SamanthaPullen Thanks for double-checking with me! Yes those changes look fine on my end, so my approval still holds.

@SamanthaPullen
Copy link
Contributor Author

@nusbaume , @mkavulich, could this PR be merged please? It has had 2 approvals, but strangely only 1 of them is showing in the panel below

@climbfuji
Copy link
Collaborator

@SamanthaPullen I'll ping folks directly to make sure this PR gets reviewed promptly. Sorry for the delay.

Copy link
Collaborator

@climbfuji climbfuji left a comment

Choose a reason for hiding this comment

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

I am in favor of adding these new names. It would be good if @mkavulich and/or @grantfirl also reviewed the PR.

Copy link
Collaborator

@mkavulich mkavulich left a comment

Choose a reason for hiding this comment

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

@SamanthaPullen thank you for making the discussed changes, sorry for the delay. Merging now.

@mkavulich mkavulich merged commit 2e46866 into ESCOMP:main Apr 11, 2024
3 checks passed
@SamanthaPullen
Copy link
Contributor Author

@mkavulich thank you:)

@grantfirl
Copy link
Collaborator

Late to the party, but looks fine to me.

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.

8 participants