Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2422 +/- ##
==========================================
- Coverage 96.18% 96.14% -0.05%
==========================================
Files 271 272 +1
Lines 15913 16089 +176
==========================================
+ Hits 15306 15468 +162
- Misses 607 621 +14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Tried with the example recipe and it looks good, output seems as expected. Fast look through the code looks good. I'll do a more detailed review when I have more time. |
|
Hello, this pull request has been marked with the If you won't be able to finish this in time, don't worry - just unassign the milestone |
|
I am sorry, it looks like this pull request won't make it into v2.14.0. If there are strong objections, please let me know. Moving this to v2.15.0. |
|
Jan-Hendrik Malles seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
Hi all, a quick note for the ESMValCore v2.15.0 release: the planned code freeze date is 19 June 2026. This PR is currently included in the v2.15.0 milestone. Could you please confirm whether it is still intended for this release and what needs to happen before it can be merged? @flicj191 tagging you for release planning visibility. |
|
Hello, I will try to get the code coverage to 100% today or tomorrow. Then someone would need to review it, I suppose. |
|
@ghossh Hey Supriyo, Jan has updated the tests now to increase the coverage. Can you have a look and do a review please? <3 |
|
Hi @bettina-gier , I'm going on holiday after today and coming back on 28th Jun. I'll take a look afterward. |
|
ah nvm then, that's after the core feature freeze where we want to have this in. I'll see if I can do it next week instead then.. |
Removed instructions for placing data files in specific subdirectories.
bettina-gier
left a comment
There was a problem hiding this comment.
It runs, results look fine. Might be nice to add a sample config for the filename even if it's in a flat folder.
@alistairsellar Is it possible to do some sort of manual CLA override? Jan signed on the github account but most commits are under a faulty config that can't sign. Otherwise we would have to somehow rebase the commits.
Hi @bettina-gier. There's no way to override the CLA list of signatories if GitHub doesn't think that a commit is linked to a GitHub account. What I would suggest is that if you know for certain that all commits in a PR have been made by people who have signed the CLA (like in this case), then you can temporarily switch off the branch protection that looks at the CLA check. (This aligns with the spirit of why we need the CLA.) And then switch the rule back on again after the PR is merged. Happy to do the switch-off when you're ready to merge, though I expect you've got permissions to change rules? |
|
I personally don't have permissions for that I believe. I'll ask for a quick final tech review before merge! |
bouweandela
left a comment
There was a problem hiding this comment.
Looks good! To wrap it up, could you please add a bit more documentation about the various facets that can be used from the recipe to control the grid and remove code that was copy/pasted from the ICON fix but does not apply to ORAS5?
| cube: iris.cube.Cube | ||
| Cube for which the ORS5 horizontal grid is retrieved. If the facet | ||
| `horizontal_grid` is not specified by the user, it raises a | ||
| NotImplementedError. |
There was a problem hiding this comment.
typically you would use a KeyError for this, or possibly a ValueError. NotImplementedError is for things that have not (yet) been implemented.
| raise NotImplementedError( | ||
| msg, | ||
| ) |
There was a problem hiding this comment.
| raise NotImplementedError( | |
| msg, | |
| ) | |
| raise NotImplementedError(msg) |
|
|
||
| # Predetermine how to handle grid | ||
| make_unstructured = self.extra_facets.get("make_unstructured", False) | ||
| u_grid = self.extra_facets.get("ugrid", False) |
There was a problem hiding this comment.
I see that there are multiple facets that may be specified in the recipe to make this data work. Could you please add documentation for those?
| raise ValueError( | ||
| msg, | ||
| ) |
There was a problem hiding this comment.
| raise ValueError( | |
| msg, | |
| ) | |
| raise ValueError(msg) |
| the ORAS5 fix is located `here | ||
| <https://github.com/ESMValGroup/ESMValCore/blob/main/esmvalcore/cmor/_fixes/native6/oras5.py>`__, |
There was a problem hiding this comment.
| the ORAS5 fix is located `here | |
| <https://github.com/ESMValGroup/ESMValCore/blob/main/esmvalcore/cmor/_fixes/native6/oras5.py>`__, |
these are intended as examples, there is no need to list everything and three examples seems a bit much
|
|
||
| def test_get_mesh_not_cached_from_facet(monkeypatch, tmp_path): | ||
| """Test fix.""" | ||
| session = CFG.start_session("my session") |
There was a problem hiding this comment.
Please use the session fixture instead.
| "horizontal_grid", | ||
| "Horizontal grid file", |
There was a problem hiding this comment.
Please add documentation on how to use these facets
|
|
||
| def fix_metadata(self, cubes: CubeList) -> CubeList: | ||
| """Fix metadata.""" | ||
| cubes = self.add_additional_cubes(cubes) |
There was a problem hiding this comment.
Is this feature needed for ORAS5?
|
|
||
| def test_add_coord_from_grid_fail_no_unnamed_dim(cubes_2d): | ||
| """Test fix.""" | ||
| # Remove latitude from tas cube to test automatic addition |
There was a problem hiding this comment.
This comment appears multiple times throughout this file but does not seem to be related to the test it is part of. Is it a result of copy/pasting another test and modifying that?
| fix = get_allvars_fix("Omon", "tos") | ||
| fix.extra_facets["frequency"] = frequency | ||
|
|
||
| fix._shift_time_coord(cube, time_coord) |
There was a problem hiding this comment.
This test appears to have been copy/pasted from
ESMValCore/tests/integration/cmor/_fixes/icon/test_icon.py
Lines 1872 to 1962 in 4f65a7b
_shift_time_coord method is only defined on the base class and not modified in the file oras5.py, there is no need to test it again. To keep the code easy to maintain, I would suggest removing it here. The same goes for the other copy/pasted tests further down that do not apply to the oras5.py file.
Description
Fix for ORAS5. It includes the option to make the 'irregular' (2d) grid 'unstructured' (1d) and UGRID-compliant. To do so, one needs to add the extra facet 'make_unstructured: true' and/or 'ugrid: true' in the recipe.
The mesh description ('horizontal_grid' in the recipe) needs to be read from a file (oras5_mesh.zip), which is based on a file that can be downloaded here. Since ORAS5 data is on the ORCA025 (Arakawa-C grid), there are different mesh files for tracers (T), and zonal/meridional velocities (u/v).
An example recipe for ORAS5 can be found here.
Some already downloaded data are stored here:
/work/bd1083/b382555/extraobsraw/Tier2/ORAS5/data
(for 3D data in all_levels so far usually only short time periods are downloaded, due to their size)
The grid files are also stored on Levante:
/work/bd1083/b382555/extraobsraw/Tier2/ORAS5/grids
Link to documentation: https://esmvaltool--2422.org.readthedocs.build/projects/ESMValCore/en/2422/quickstart/find_data.html#oras5
Before you get started
Checklist
It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.
To help with the number pull requests: