Repository navigation
MAINT: Some cleanup of convert.py - #2964
Merged
VeckoTheGecko merged 17 commits intoOct 23, 2026
Merged
Conversation
nemo_to_sgrid and copernicusmarine_to_sgrid negate W with `.data *= -1`, which modifies the caller's array in place.
Safety net for refactoring parcels.convert. tests/_legacy_convert.py is a verbatim copy of convert.py; test_convert_legacy.py runs 42 input cases through both and requires identical output datasets, errors, log messages and warnings. Inputs reuse existing tutorial, remote and synthetic datasets plus variations of them targeting otherwise unexercised branches, raising branch coverage of convert.py from 69% to 89% (the rest is dead or unreachable code). Both files are meant to be deleted once the refactor lands.
- Remove _discover_U_and_V and _ds_rename_using_standard_names, which had no callers since field names became an explicit argument - Remove duplicate entries from _NEMO_DIMENSION_COORD_NAMES (only used for membership checks and idempotent loops, so no behaviour change) - Document the actual fields/coords parameters of nemo_to_sgrid - Fix grammar in the module docstring No behaviour change: tests/test_convert_legacy.py passes unchanged.
Pull the code duplicated across the SGRID converters into helpers: - _normalize_fields: turn each entry of `fields` into a DataArray named after its key (previously copy-pasted in all 5 converters) - _attach_sgrid_grid: add the 'grid' variable with 2D SGRID metadata (previously copy-pasted in all 5 converters) - _flip_w_to_positive_down: negate W (previously in NEMO and Copernicus) No behaviour change: tests/test_convert_legacy.py passes unchanged. In particular, _flip_w_to_positive_down still modifies W in place, and _attach_sgrid_grid does not run _assert_no_grid_metadata (Delft3D never called it). Both are left for a follow-up that intentionally changes behaviour.
- Add a _ModelConventions dataclass holding a model's expected coords, variable renames and axis attributes, and use it for NEMO, MITgcm, CROCO and Delft3D instead of 3-4 loose module constants each. NEMO's list of kept dimensions and Copernicus Marine's rename-by-CF-axis mapping stay separate constants, as no other model uses them. - Move the FESOM2/ICON vertical dimension constants up with the others. - Split _maybe_bring_other_depths_to_depth, which did two unrelated things, into _rename_nemo_depth_dims and _nemo_maybe_expand_depth_dimension. The latter is not yet merged with _maybe_expand_depth_dimension, as they insert the depth dimension at a different position and log different messages. No behaviour change: tests/test_convert_legacy.py passes unchanged.
nemo_to_sgrid and copernicusmarine_to_sgrid negated W with `.data *= -1`, which changed the user's input array in place. Negate into a new array instead.
_assert_no_grid_metadata looked for a "grid" key in ds.cf.cf_roles, but that mapping is keyed by cf_role value, so existing grid metadata (cf_role "grid_topology") was never detected. Check for "grid_topology" instead, and run the check from _attach_sgrid_grid so that every converter does it (delft3d_to_sgrid previously didn't).
nemo_to_sgrid and delft3d_to_sgrid kept the global attributes that xr.merge copied over from the first field, whereas the other SGRID converters cleared them. Clear them in all converters, and attach the grid through parcels._sgrid._attach_sgrid_metadata so that the output also has Conventions="SGRID", as Parcels' own SGRID datasets do.
NEMO and Copernicus Marine each had their own helper to add a singleton depth dimension to surface data. They logged different messages, and NEMO inserted depth before time while Copernicus inserted it after. Use a single helper for both, which puts depth after time (the order Parcels transposes fields to anyway). Unlike expand_dims(axis=1), this also works for data variables with fewer than two dimensions.
Each value in `fields` may be a Dataset, from which the variable named after the field is selected. This was undocumented, and a Dataset without such a variable raised a bare KeyError. Document it in the docstrings of the SGRID converters and raise a ValueError explaining the fix instead. Also use model-specific variable names in the docstring examples, and don't mark `coords` as optional for mitgcm_to_sgrid and croco_to_sgrid, where it is required.
Move the check for existing grid metadata from parcels.convert into parcels._sgrid._attach_sgrid_metadata, so it applies wherever SGRID metadata is attached. The convert functions now call _attach_sgrid_metadata directly (as the rest of the codebase does), which makes their _attach_sgrid_grid wrapper unnecessary.
erikvansebille
left a comment
erikvansebille
left a comment
Member
There was a problem hiding this comment.
I think this already looks quite nice! See below some first comments/suggestions
6 tasks done
# Conflicts: # src/parcels/convert.py # tests/test_convert.py
Re-adds the MOM6 converter and tests from upstream (Parcels-code#2935), which were left out when resolving the merge conflicts. Uses the shared helpers (_ModelConventions, _normalize_fields, _attach_sgrid_metadata) and clears global attributes like the other SGRID converters.
… copy Parcels-code#2958 changed the CROCO SGRID padding in croco_to_sgrid from HIGH to NONE. Mirror that in the frozen legacy copy so test_convert_legacy.py compares against the current behaviour on main.
Contributor
Author
|
I've gone and fixed the merge conflicts here. The behaviour in the old convert and new convert file are equal, so I've cleaned up the temporary files (58b9cc7 ). Is this good to merge now @erikvansebille ? |
VeckoTheGecko
marked this pull request as ready for review
October 22, 2026 12:50
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Refactoring of the
convert.pymodule.(will discuss more in-person)
Pre-merge TODO:
Checklist
mainfor normal development,v3-supportfor v3 support)AI Disclosure