MetOffice / opsinputs

JEDI library generating VarObs and Cx files
BSD 3-Clause "New" or "Revised" License
4 stars 0 forks source link

Configurable model potential temperature name #151

Closed ctgh closed 1 year ago

ctgh commented 1 year ago

Make the model potential temperature name configurable in the Cx Writer. The default (theta) will be used for ops-um-jedi, but it can be changed to potential_temperature for LFRic.

If this solution is accepted then it should be validated in larger tests (including sith for a null test).

ctgh commented 1 year ago

@ss421 This is a draft PR because I just tested it locally in a unit test. Is there a way that you could try it for a larger case?

ctgh commented 1 year ago

@mikecooke77 Thanks for your review. I agree that if it proliferates for a lot of variables that would be very difficult to maintain. Do you know roughly how many variables have different names?

mikecooke77 commented 1 year ago

I know there is also an issue with bulk area vs bulk volume cloud fraction. I just think on principal consistent GeoVaLs should be being produced.

ctgh commented 1 year ago

I have added some more extensive testing in ae58fac. That revealed that I had to change "theta" in two more places in the code.

I have no strong opinions as to whether this should be merged. If it would be better to pursue unifying the GeoVaL names that's fine. I believe they will eventually have to be unified once JCSDA decide on a naming convention.

ss421 commented 1 year ago

The following fields have different names in lfric-lite compared to ops-um-jedi:

liquid_cloud_volume_fraction_in_atmosphere_layer -- liquid_cloud_fraction ice_cloud_volume_fraction_in_atmosphere_layer -- frozen_cloud_fraction potential_temperature -- theta

and the following don't seem to have variable names in lfric-lite. So maybe that means we can just add them:

cloud_area_fraction_in_atmosphere_layer surface_pressure total_cloud_amount

and dust is not yet available. For some variables, there will be a variable change.

I added a spreadsheet with the details: see here

ctgh commented 1 year ago

So one possible solution would be to have three of these parameters until the names are unified?

Can I just check what the names in red are in the spreadsheet?

mikecooke77 commented 1 year ago

The problem with these temporary fixes is they tend to hang around. The differences Steve mentions could also be a reason why some of the ObsOperators are not working with lfric-lite-jedi. Given that this code should be agnostic to the model I would much rather an agreement on model names was reached.

mikecooke77 commented 1 year ago

The following fields have different names in lfric-lite compared to ops-um-jedi:

liquid_cloud_volume_fraction_in_atmosphere_layer -- liquid_cloud_fraction ice_cloud_volume_fraction_in_atmosphere_layer -- frozen_cloud_fraction potential_temperature -- theta

and the following don't seem to have variable names in lfric-lite. So maybe that means we can just add them:

cloud_area_fraction_in_atmosphere_layer surface_pressure total_cloud_amount

and dust is not yet available. For some variables, there will be a variable change.

I added a spreadsheet with the details: see here

Is this worth raising as an issue in lfric-lite-jedi?

ctgh commented 1 year ago

I tend to agree, but just to play devil's advocate, once the model naming conventions are agreed I assume that we will have to make various changes to opsinputs anyway, in which case the temporary fixes can be removed. That is speculation though! If Steve agrees I will close this for now and potentially reopen it if there are issues with the interface.

mikecooke77 commented 1 year ago

Fair point @ctgh. I would suggest leave this as a draft for now until we have a resolution on the model names.

ss421 commented 1 year ago

So one possible solution would be to have three of these parameters until the names are unified?

Can I just check what the names in red are in the spreadsheet?

They are variables that don't directly map - there will be a variable change for it but I have not yet look at that. I can check about those.

mikethurlow commented 1 year ago

I could add that we also get failures about missing fields that don't appear in either list. Take for example scatwind: driven by UM it works, driven by lfric it fails with

lfriclitejedi::Table::extractValue :: geopotential_height_levels_minus_one key not part of table

eg failure message

Table.cc probably needs updating to add this variable.

ctgh commented 1 year ago

Based on the recent email conversations I think it's OK to close this now?

ctgh commented 1 year ago

Closing given the changes made in lfric-lite-jedi and related repositories.