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

Feature/adding dart support #246

Merged
merged 2 commits into from
Oct 2, 2024

Conversation

stefaniereuter
Copy link
Contributor

This is related to ECC-1922 , to add support for Fesom grid DART . This is relevant for intermediate climate DT runs with FESOM.

@FussyDuck
Copy link

FussyDuck commented Sep 20, 2024

CLA assistant check
All committers have signed the CLA.

@stefaniereuter
Copy link
Contributor Author

I initially thought that C and N stands for Core2 and NG5 in subgridType but I then found a comment from Willem that it actually stands for centroids and edges i.e. the second commit that removes the D

@shahramn
Copy link
Collaborator

@stefaniereuter You need to sign the CLA (Contributor License Agreement) before we can proceed.
Vielen Dank

@dsarmany
Copy link

I initially thought that C and N stands for Core2 and NG5 in subgridType but I then found a comment from Willem that it actually stands for centroids and edges i.e. the second commit that removes the D

Yes, that is correct. So the combined changes in this PR are fine. It will be probably be worth squashing them into a single commit at the point of merging it.

@stefaniereuter
Copy link
Contributor Author

Hi Sharam, sorry I had already signed it but I did not click on rechecking. For me it now says all committers signed... So I assume it worked

@shahramn
Copy link
Collaborator

shahramn commented Sep 23, 2024

The last three entries in this table should be prefixed by a string like FESOM
e.g.,

'FESOM_CORE2' = { numberOfGridUsed = 8; }
'FESOM_NG5'   = { numberOfGridUsed = 9; }
'FESOM_DART' =  { numberOfGridUsed = 10; }

Note how the other names have the ORCA substring.

Really this kind of naming should be the responsibility of Data Governance

@stefaniereuter
Copy link
Contributor Author

I've just contacted Data Governance. I'll create a ticket in their jira space and will let you know what they say.

@mjg41 mjg41 self-requested a review September 25, 2024 14:11
Copy link
Collaborator

@mjg41 mjg41 left a comment

Choose a reason for hiding this comment

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

Hi,
From discussions with @wdeconinck and his confluence page that you linked above, all those new ocean grids are handcrafted for the FESOM ocean model by the AWI developers. Therefore I'd be happy with leaving them as they are with no additional changes to be honest. Ultimately this is only a concept helper key which is in our local definitions in any case, and the number is the key fixed piece of metadata.
So now DGOV approved, thanks for bringing it to our attention!

@shahramn shahramn self-assigned this Oct 2, 2024
@shahramn shahramn added the approved-for-ci Approved to run CI on ECMWF machines label Oct 2, 2024
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.81%. Comparing base (344d9ef) to head (f917841).
Report is 12 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #246   +/-   ##
========================================
  Coverage    87.81%   87.81%           
========================================
  Files          776      776           
  Lines        62512    62512           
  Branches     11035    11035           
========================================
  Hits         54892    54892           
  Misses        7620     7620           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@shahramn
Copy link
Collaborator

shahramn commented Oct 2, 2024

We have a metkit test failure (metkit_test_codes_decoder). The test expects the value of "unknown" for unstructuredGridType.
I have asked @danovaro to fix it. Once that is done, I will merge this PR

@shahramn shahramn merged commit db04edb into ecmwf:develop Oct 2, 2024
153 of 193 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved-for-ci Approved to run CI on ECMWF machines contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants