Skip to content

Remove XGrid.from_dataset#2648

Merged
VeckoTheGecko merged 12 commits into
Parcels-code:mainfrom
VeckoTheGecko:push-nqqktyrqvskz
May 29, 2026
Merged

Remove XGrid.from_dataset#2648
VeckoTheGecko merged 12 commits into
Parcels-code:mainfrom
VeckoTheGecko:push-nqqktyrqvskz

Conversation

@VeckoTheGecko
Copy link
Copy Markdown
Contributor

@VeckoTheGecko VeckoTheGecko commented May 28, 2026

Description

This code path isn't used by from_sgrid_conventions and was mainly used before we had SGRID support by piggy-backing off xgcm functionality.

This PR removes this classmethod and updates the test suite. Some parts of the test suite haven't been updated and have instead been flagged (as they're likely to be removed in a future PR).

This PR is blocked by #2647 (once this PR is merged this will be ready for review)

Checklist

AI Disclosure

None used

@VeckoTheGecko VeckoTheGecko marked this pull request as ready for review May 29, 2026 08:18
@VeckoTheGecko
Copy link
Copy Markdown
Contributor Author

VeckoTheGecko commented May 29, 2026

Hmmmmm, not sure why the conda package build process is failing

 │ │
 │ │ ╭─ Installing test environment
 │ │ │ ✔ Successfully updated the test environment
 │ │ │
 │ │ ╰─────────────────── (took 14 seconds)
 │ │ Matplotlib is building the font cache; this may take a moment.
 │ │ $SRC_DIR/conda_build_script.py:1: UserWarning: This is an alpha version of Parcels v4. The API is not stable and may change without deprecation warnings.
 │ │   import parcels
 │ │ ✔ python imports test passed!
 │ │ panel 1.9.2 requires panel-material-ui, which is not installed.
 │ │ × error Script failed with status 1
 │ │ × error 
 │ │ × error Script execution failed.
 │ │ × error 
 │ │ × error   Work directory: /var/folders/j5/x0w7f5sn7gg25__6pg48j82c0000gn/T/.tmpVuU6ZK/parcels-0.1.0dev-pyh4616a5c_0
 │ │ × error   Prefix: /Users/Hodgs004/coding/repos/parcels/output/test/test_parcelsOYpBf8/test_env
 │ │ × error   Build prefix: None
 │ │ × error 
 │ │ × error To run the script manually, use the following command:
 │ │ × error 
 │ │ × error   cd "/var/folders/j5/x0w7f5sn7gg25__6pg48j82c0000gn/T/.tmpVuU6ZK/parcels-0.1.0dev-pyh4616a5c_0" && ./conda_build.sh
 │ │ × error 
 │ │ × error To run commands interactively in the build environment:
 │ │ × error 
 │ │ × error   cd "/var/folders/j5/x0w7f5sn7gg25__6pg48j82c0000gn/T/.tmpVuU6ZK/parcels-0.1.0dev-pyh4616a5c_0" && source build_env.sh
 │ │
 │ ╰─────────────────── (took 74 seconds)
 │
 ╰─────────────────── (took 74 seconds)

This is happening on main as well - likely a problem with a dependency in the build environment.... Not a blocker for this PR

Copy link
Copy Markdown
Member

@erikvansebille erikvansebille left a comment

Choose a reason for hiding this comment

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

Looks good; two minor questions below

Comment thread tests/conftest.py
Comment on lines +29 to +34
ds = ds[["U_A_grid", "V_A_grid", "grid"]].rename(
{
"U_A_grid": "U",
"V_A_grid": "V",
}
)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need to worry about the values in the arrays of these datasets? For example, if they are by default zero then some advection tests may pass irrespective of how good the advection scheme is. Or is it simply not intended that the values in these arrays are used?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Discussed in person - these datasets have random data at the moment, where other datasets can be used for more flow specific tests.

Comment thread tests/conftest.py


@pytest.fixture
def fieldset() -> FieldSet:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's a bit 'magical' that the fieldset() is defined here, but can be used in all the test files without an explicit import in these test files. New devs may get confused?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Discussed in person - this is just pytest magic :)

@VeckoTheGecko VeckoTheGecko merged commit b07151b into Parcels-code:main May 29, 2026
16 of 17 checks passed
@github-project-automation github-project-automation Bot moved this from Backlog to Done in Parcels development May 29, 2026
@VeckoTheGecko VeckoTheGecko deleted the push-nqqktyrqvskz branch May 29, 2026 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants