Skip to content

Add axes and get_axis_dim to BaseGrid and other cleanup #2080

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

Merged
merged 12 commits into from
Jul 11, 2025

Conversation

VeckoTheGecko
Copy link
Contributor

Changes:

  • Added axes and get_axis_dim() to the BaseGrid class with corresponding implementations in the UxGrid and XGrid classes. Also moved ravel/unravel logic to the BaseGrid class (fixes Adding axes and get_axis_dim to BaseGrid #2056)
  • Removed lon, lat, depth from the Field class
    • These attributes didn't really make sense in the unstructured context (especially now we've settled on a different API), and also weren't used in the codebase. It makes more sense for these to live on the grids anyway where needed.
  • Add uxarray to pyproject.toml dependencies (25c589b)
  • Other cleanup (see commit messages)

Open questions:

  • @fluidnumerics-joe, the implementation of Field.depth differed from UxGrid.depth - with the former using a helper function get_vertical_location_from_dims. Is this function still needed, or can we remove it?

  • Chose the correct base branch (main for v3 changes, v4-dev for v4 changes)

  • Fixes Adding axes and get_axis_dim to BaseGrid #2056

  • Added tests

  • Added documentation

Wasn't used anymore in the codebase
Overloaded behaviour that wasn't justified/used in the rest of the codebase.
Was in environment.yaml file - wasn't in the pyproject.toml file though
These variables mainly apply to structured grids. With the new API, they aren't needed in the codebase (we can add this on the repective grid classes if needed).
Also added some simple tests in test_uxgrid.py
@abstractmethod
def get_axis_dim(self, axis: str) -> int:
"""
Return the dimensionality (number of cells/edges) along a specific axis.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
Return the dimensionality (number of cells/edges) along a specific axis.
Return the dimensionality (number of cells/faces) along a specific axis.

I think that this should be cells/faces and not 'edges'. This is what we ravel against - so edges don't really apply (since particles can't occupy that)

@fluidnumerics-joe
Copy link

fluidnumerics-joe commented Jul 10, 2025

@fluidnumerics-joe, the implementation of Field.depth differed from UxGrid.depth - with the former using a helper function get_vertical_location_from_dims. Is this function still needed, or can we remove it?

We can remove the helper function now that we have the understanding in UxGrid that the vertical position is explicitly relative to the vertical layers.

Copy link
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; one question below

@VeckoTheGecko VeckoTheGecko enabled auto-merge (rebase) July 11, 2025 10:55
@VeckoTheGecko VeckoTheGecko merged commit b7bdddc into v4-dev Jul 11, 2025
8 of 9 checks passed
@VeckoTheGecko VeckoTheGecko deleted the cleanup-big-picture branch July 11, 2025 10:57
@github-project-automation github-project-automation bot moved this from Backlog to Done in Parcels development Jul 11, 2025
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.

3 participants