Skip to content

Make variable names conform for ufo-data - #312

Merged
shlyaeva merged 11 commits into
developfrom
feature/make_varnames_agree_part1
Apr 27, 2023
Merged

shlyaeva merged 11 commits into
developfrom
feature/make_varnames_agree_part1

Conversation

@mo-keithstewart

@mo-keithstewart mo-keithstewart commented Mar 16, 2023 •

Copy link
Copy Markdown
Contributor

Description

This is the work required for issue #311 for changes to the variable names so that there is more consistency in the system, as defined in this spreadsheet

The following fields are updated:
theta >> potential_temperature
cloud_area_fraction_in_atmosphere_layer >> cloud_volume_fraction_in_atmosphere_layer
liquid_cloud_fraction >> liquid_cloud_volume_fraction_in_atmosphere_layer
frozen_cloud_fraction >> ice_cloud_volume_fraction_in_atmosphere_layer

This fixes an issue we currently have with variable mismatch between different parts of the system.

(NB: two of the data files, abi_g16_geoval_2018041500_m_qc.nc4 and pace_radiance_geovals_2019032112.nc, have been reverted to their original versions to retain the variable cloud_area_fraction_in_atmosphere_layer)

Acceptance Criteria (Definition of Done)

Test builds and run

Test output is here.

Dependencies

Requires changes to ops-um-jedi, opsinputs, ufo, lfric-lite-jedi, saber, vader and sith.

  • waiting on ops-um-jedi: #187
  • waiting on opsinputs: #163
  • waiting on ufo: #2719
  • waiting on lfric-lite-jedi: #243
  • waiting on saber: #502
  • waiting on vader: #162
  • waiting on sith: #174

@mo-keithstewart

Copy link
Copy Markdown
Contributor Author

Note: all ctests are passing on SPICE and CRAY_GNU. However there is an issue with the make process on CRAY_INTEL and CRAY_INTEL_DEBUG. I have contacted Matt Shin about this and he is investigating.

@mikecooke77

Copy link
Copy Markdown
Collaborator

@BenjaminTJohnson there are quite a few files associated with CRTM that are changed as part of this PR. Do you think these are sensible changes? I'm thinking in particular about cloud_area_fraction_in_atmosphere_layer >> cloud_volume_fraction_in_atmosphere_layer

@BenjaminTJohnson

Copy link
Copy Markdown
Collaborator

@mikecooke77 I potentially disagree with these changes:

cloud_area_fraction_in_atmosphere_layer >> cloud_volume_fraction_in_atmosphere_layer
liquid_cloud_fraction >> liquid_cloud_volume_fraction_in_atmosphere_layer
frozen_cloud_fraction >> ice_cloud_volume_fraction_in_atmosphere_layer

Strictly speaking, these are not volume fractions, but area fractions. I suppose one could consider these as volume fractions, but it's not precisely true. We provide water "content" as a layer water path (kg/m^2).

I can be convinced on this.

@mikecooke77

Copy link
Copy Markdown
Collaborator

@mikecooke77 I potentially disagree with these changes:

cloud_area_fraction_in_atmosphere_layer >> cloud_volume_fraction_in_atmosphere_layer
liquid_cloud_fraction >> liquid_cloud_volume_fraction_in_atmosphere_layer
frozen_cloud_fraction >> ice_cloud_volume_fraction_in_atmosphere_layer

Strictly speaking, these are not volume fractions, but area fractions. I suppose one could consider these as volume fractions, but it's not precisely true. We provide water "content" as a layer water path (kg/m^2).

I can be convinced on this.

I thought this was the case. In which case we should have both:
cloud_area_fraction_in_atmosphere_layer and cloud_volume_fraction_in_atmosphere_layer. The same for the other two e.g.
liquid_cloud_area_fraction_in_atmosphere_layer and liquid_cloud_volume_fraction_in_atmosphere_layer
ice_cloud_area_fraction_in_atmosphere_layer and ice_cloud_volume_fraction_in_atmosphere_layer
@jameshocking-mo which is most accurate from an RTTOV point of view?

@jameshocking-mo

Copy link
Copy Markdown

@mikecooke77 As a bit of an outsider, it's not obvious to me that it's a good idea to rename volume fractions as area fractions, or vice versa, since the two are different. Having discussed this with Cyril Morcrette in the past, the area fraction is most appropriate for use with RTTOV, but in its absence bulk/volume fraction can be used.

@mikecooke77 mikecooke77 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @jameshocking-mo and @BenjaminTJohnson for the comments. @mo-keithstewart It looks like we shouldn't just be renaming these variables. My suggestion is we have both area and volume fractions for the cloud variables. This will require both to be in the ufo_variables_mod and these should not be being renamed in this ufo-data PR. It will be down to the individual observation operators to decide which one to use. This clearly has implications for all linked PRs.

@ss421

ss421 commented Mar 30, 2023

Copy link
Copy Markdown

The names adopted here are documented in the excel spreadsheet which I can share on teams. Here we have adopted the agreed names in that sheet. That was based on conversations with people in the Met Office and we didn't get any objections. The proposed names were originally included in lfric-lite-jedi and I think @stemiglio added them.

To clarify, the suggestion is to have the following:

cloud_area_fraction_in_atmosphere_layer [stash code 265]
cloud_volume_fraction_in_atmosphere_layer [stash code 266]
liquid_cloud_fraction (and not liquid_cloud_volume_fraction_in_atmosphere_layer) [stash code 267]
frozen_cloud_fraction (and not ice_cloud_volume_fraction_in_atmosphere_layer) [stash code 268]

Is that correct - if so I can update the spreadsheet. It is my understanding that RTTOV uses the field associated with stash code 266. We do not output stash code 265 in our operational configuration.

If we adopted two names as suggested above, how do we create the ufo geoval data that is required for the additional variable? Who created the data to begin with? If it is us then it makes sense to rename the data to cloud_volume_fraction_in_atmosphere_layer but I'm not sure what to do about the other geoval data.

(some further discussion on the cloud_area_fraction_in_atmosphere_layer and cloud_volume_fraction_in_atmosphere_layer is given in https://github.com/JCSDA-internal/ufo/issues/2627.

@mikecooke77

Copy link
Copy Markdown
Collaborator

If you look at the final comments on https://github.com/JCSDA-internal/ufo/issues/2627. There was the suggested need for two variables one called area and one called volume. If we stick with what is currently in ufo_variables_mod and add new variables for volume that does not affect those using CRTM and those associated model interfaces. The change as is will affect all model interfaces outside of the Met Office. We will have to make some changes in RTTOV but these can be done shortly after these changes or as part of it.

@mikecooke77

Copy link
Copy Markdown
Collaborator

cloud_area_fraction_in_atmosphere_layer [stash code 265] > If we adopted two names as suggested above, how do we create the ufo geoval data that is required for the additional variable? Who created the data to begin with? If it is us then it makes sense to rename the data to cloud_volume_fraction_in_atmosphere_layer but I'm not sure what to do about the other geoval data.

To answer this question specifically. We would use volume from the model interfaces and we would not create area. The RTTOV interface will need to change to expect the new variable name. Other model interfaces would still produce area values for use with CRTM.

@ss421

ss421 commented Mar 30, 2023

Copy link
Copy Markdown

If we are adopting cloud_volume_fraction_in_atmosphere_layer in the interface then we need to make the RTTOV change as part of this I think?

In terms of the ufo data - we leave the ufo data as it is but create another one for cloud_volume_fraction_in_atmosphere_layer? Who would be best placed to advise on how to do that?

@mikecooke77

Copy link
Copy Markdown
Collaborator

For the testing the RTTOV geoval file can just be renamed because area and volume fraction are very similar values. The CRTM files will need to be reverted. As you say the RTTOV interface will need changing and I'm happy to discuss this with Keith. This will also affect the other associated PRs in other ways such-as opsinputs.

@mikecooke77

Copy link
Copy Markdown
Collaborator

I think this is also an issue for:
liquid_cloud_volume_fraction_in_atmosphere_layer
ice_cloud_volume_fraction_in_atmosphere_layer

@ss421

ss421 commented Mar 30, 2023

Copy link
Copy Markdown

For the testing the RTTOV geoval file can just be renamed because area and volume fraction are very similar values. The CRTM files will need to be reverted. As you say the RTTOV interface will need changing and I'm happy to discuss this with Keith. This will also affect the other associated PRs in other ways such-as opsinputs.

Thanks @mo-keithstewart said that he has setup a meet - thanks.

@ss421

ss421 commented Mar 30, 2023

Copy link
Copy Markdown

I think this is also an issue for: liquid_cloud_volume_fraction_in_atmosphere_layer ice_cloud_volume_fraction_in_atmosphere_layer

What is the issue? My reading from this PR is that we should not change the name, so we will have:

liquid_cloud_fraction (and not liquid_cloud_volume_fraction_in_atmosphere_layer) [stash code 267]
frozen_cloud_fraction (and not ice_cloud_volume_fraction_in_atmosphere_layer) [stash code 268]

Is that not correct?

@BenjaminTJohnson BenjaminTJohnson left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Some geovals files end in .nc, others end in .nc4. I think it should be made consistent.

@mikecooke77

Copy link
Copy Markdown
Collaborator

I think this is also an issue for: liquid_cloud_volume_fraction_in_atmosphere_layer ice_cloud_volume_fraction_in_atmosphere_layer

What is the issue? My reading from this PR is that we should not change the name, so we will have:

liquid_cloud_fraction (and not liquid_cloud_volume_fraction_in_atmosphere_layer) [stash code 267] frozen_cloud_fraction (and not ice_cloud_volume_fraction_in_atmosphere_layer) [stash code 268]

Is that not correct?

I think the um and lfric mapping is correct. However for CRTM they will need liquid_cloud_area_fraction_in_atmosphere_layer and ice_cloud_area_fraction_in_atmosphere_layer. So in ufo it makes sense to map the current names to area and then its a smaller change for CRTM. New volume equivalents will be add in the ufo_variables_mod and then RTTOV would use those for now.

@mo-keithstewart

Copy link
Copy Markdown
Contributor Author

I have added in the changes created by Mike that affect two of the data files, which required to be reverted back to their original versions.

@mikecooke77

Copy link
Copy Markdown
Collaborator

Some geovals files end in .nc, others end in .nc4. I think it should be made consistent.

I agree this would be good to do. I think it will probably require a separate PR and a decision to be made on a naming convention.

@mikecooke77 mikecooke77 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Changes look good to me.

@mo-keithstewart
mo-keithstewart marked this pull request as ready for review April 18, 2023 15:18

@PatNichols PatNichols left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks. It looks good.

@mo-keithstewart mo-keithstewart added the waiting for other repos waiting on update in the other repos (models) label Apr 19, 2023

@ss421 ss421 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ran mo-bundle all tests are passing and can be seen: here. Thanks,

@ss421 ss421 added coordinate merge do not merge Something is wrong, do not merge labels Apr 19, 2023
@ss421 ss421 removed the do not merge Something is wrong, do not merge label Apr 26, 2023
@shlyaeva
shlyaeva merged commit 7471536 into develop Apr 27, 2023
@shlyaeva
shlyaeva deleted the feature/make_varnames_agree_part1 branch April 27, 2023 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting for other repos waiting on update in the other repos (models)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants