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

Fix MARS key duplicates for records with levtype=heightAboveSea #6

Open
wants to merge 2 commits into
base: revise_mars_model
Choose a base branch
from

Conversation

victoria-cherkas
Copy link
Collaborator

@victoria-cherkas victoria-cherkas commented Oct 18, 2023

typeOfLevel = (depthBelowLand, heightAboveSea) were interpreted as marsLevType = sfc, which meant there was no level (and theremore mars.levelist) defined. We require the level since we have multiple records with different scaledValueOfFirstFixedSurface etc.
Changes marsLevType from sfc to ml for both (depthBelowLand, heightAboveSea).

This resolves the issue of duplicates for heightAboveSea , as records now have various level values.
This does not resolve the issue of duplicates for depthBelowLand, since the resulting level is 0 for multiple records (eg 0.01 rounds to 0, 0.05 rounds to 0 etc).

@victoria-cherkas
Copy link
Collaborator Author

Issue likely related to ecmwf/earthkit-data#121

@cosunae
Copy link
Owner

cosunae commented Nov 3, 2023

Hi @victoria-cherkas

I see the point, and I also do not know why depthBelowLand, heightAboveSea are mapped into sfc.
But they are implemented as sfc in the "official" mars mapping of eccodes, so shouldnt we ask how is this working at ECMWF with their data ? We could add it as a topic to discuss next week, and keep it open until we understand why this is not a problem for their data ?

@victoria-cherkas
Copy link
Collaborator Author

victoria-cherkas commented Nov 3, 2023

Fine for me to clarify with ECMWF first, but I think the issue comes from the eccodes_cosmo_definitions, so perhaps ECMWF still has the mars.levelist defined for 'sfc' fields.
https://github.com/COSMO-ORG/eccodes-cosmo-resources/blob/master/definitions/grib2/template.4.horizontal.def#L83-L85

Perhaps this PR should rather remove those three lines of code, but not sure if that would have negative side effects.

Update - didnt realise that file in eccodes cosmo definition is just copied from eccodes definitions, so actually yes they should have the same issue. https://github.com/ecmwf/eccodes/blob/2.25.1/definitions/grib2/template.4.horizontal.def#L80-L82

@cosunae
Copy link
Owner

cosunae commented Nov 10, 2023

@petrabaumann was this discussed during these days ? I think you discussed a new mars levtype encoding for height above sea and height about ground ?

@petrabaumann
Copy link
Collaborator

Yes, I started the discussion on that, and Sebastien told me that they had to add level types for Destination Earth, e.g. a height-above-ground-type hl. These changes for Destination Earth are available in eccodes 2.32.0. I will check these changes and discuss how to adopt them with Sebastien in a follow-up.

Copy link
Collaborator

@petrabaumann petrabaumann left a comment

Choose a reason for hiding this comment

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

Thanks for spotting this!
I think that we should introduce new type of level values to differentiate between model levels, height above mean sea, and depth below land levels. ECMWF also started to work on these issues for Detination Earth.

@@ -46,7 +46,7 @@ alias grib2LocalSectionNumber=localDefinitionNumber;

if (centreForLocal == 215) {
# class
transient marsClass='od';
# transient marsClass='od';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can be removed.

@@ -13,13 +13,15 @@
'pl' = {typeOfFirstFixedSurface=100; typeOfSecondFixedSurface=255;}
'pl' = {typeOfFirstFixedSurface=100; typeOfSecondFixedSurface=100;}
'sfc' = {typeOfFirstFixedSurface=101; typeOfSecondFixedSurface=255;}
'sfc' = {typeOfFirstFixedSurface=102; typeOfSecondFixedSurface=255;}
# heightAboveSea
'ml' = {typeOfFirstFixedSurface=102; typeOfSecondFixedSurface=255;}
'sfc' = {typeOfFirstFixedSurface=103; typeOfSecondFixedSurface=255;}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would introduce antother key value 'zl' for this level type. I am planning to add also a height above ground level type 'hl', with rules for special meteorologocal parameters as 2m-temperature, 10m wind components, as ECMWF introduced for Destination Earth in another PR.

'sfc' = {typeOfFirstFixedSurface=106;typeOfSecondFixedSurface=106;}
# depthBelowLand
'ml' = {typeOfFirstFixedSurface=106; typeOfSecondFixedSurface=255;}
'ml' = {typeOfFirstFixedSurface=106;typeOfSecondFixedSurface=106;}
'pt' = {typeOfFirstFixedSurface=107; typeOfSecondFixedSurface=255;}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should define a special value for soil levels here, e.g. 'sl' or something more expressive. Can discuss that with Sebastien, when we review our model.I still have to figure out how to solve the scaling issue.

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.

3 participants