Skip to content

New grid object for structured grids#1966

Merged
VeckoTheGecko merged 39 commits intov4-devfrom
v/new-grid
Apr 15, 2025
Merged

New grid object for structured grids#1966
VeckoTheGecko merged 39 commits intov4-devfrom
v/new-grid

Conversation

@VeckoTheGecko
Copy link
Contributor

@VeckoTheGecko VeckoTheGecko commented Apr 8, 2025

Currently making this PR for visibility and so that we can align our approaches between #1946 and this.

Note that this PR only adds the new Grid object (along with a GridAdapter object) but doesn't use them in the larger codebase. That will be for future development.

Just to clarify, I'm not 100% sure if GridAdapter will make it into the final v4 and can't even particularly comment on its usefulness due to the change of state/grid manipulation done by the rest of the codebase. It might be better to bring the rest of the codebase to work with the new Grid object.

  • Chose the correct base branch (v4-dev for v4 changes)
  • [] Fixes #
  • Added tests
  • Added documentation

cc @fluidnumerics-joe

@fluidnumerics-joe
Copy link
Contributor

@VeckoTheGecko - Do we really want to maintain a duplicate of code already provided by xgcm ? ie, why wouldn't we want to just use xgcm as a dependency ?

@VeckoTheGecko
Copy link
Contributor Author

@VeckoTheGecko - Do we really want to maintain a duplicate of code already provided by xgcm ? ie, why wouldn't we want to just use xgcm as a dependency ?

Oh yes, I should have mentioned this in the issue body. I think we should vendor as opposed to adding it as a dependency for the following reasons:

  1. xgcm is quite a specific tool (i.e., enabling grid aware calculus on ocean grids). We only want the "grid aware" part, which (a) is not particularly part of the public API, and (b) is a relatively small part of their codebase . Depending on xgcm would couple us to their development, and relying on their private API would be potentially unstable. Also they're looking to get rid of the Axis object from the public API Lets get rid of Axis xgcm/xgcm#405 , which I think further would make things difficult for us.

@fluidnumerics-joe
Copy link
Contributor

@VeckoTheGecko - Do we really want to maintain a duplicate of code already provided by xgcm ? ie, why wouldn't we want to just use xgcm as a dependency ?

Oh yes, I should have mentioned this in the issue body. I think we should vendor as opposed to adding it as a dependency for the following reasons:

  1. xgcm is quite a specific tool (i.e., enabling grid aware calculus on ocean grids). We only want the "grid aware" part, which (a) is not particularly part of the public API, and (b) is a relatively small part of their codebase . Depending on xgcm would couple us to their development, and relying on their private API would be potentially unstable. Also they're looking to get rid of the Axis object from the public API Lets get rid of Axis xgcm/xgcm#405 , which I think further would make things difficult for us.

All good reasons.

@VeckoTheGecko
Copy link
Contributor Author

For context as well xgcm/xgcm#658

@VeckoTheGecko
Copy link
Contributor Author

I think that this is ready for review. Post-merge we can work out how to integrate this into Field, and decide if GridAdapter is needed after all

@VeckoTheGecko VeckoTheGecko marked this pull request as ready for review April 10, 2025 12:46
@VeckoTheGecko VeckoTheGecko disabled auto-merge April 15, 2025 09:34
@VeckoTheGecko VeckoTheGecko merged commit 1316f7a into v4-dev Apr 15, 2025
14 of 15 checks passed
@VeckoTheGecko VeckoTheGecko deleted the v/new-grid branch April 15, 2025 09:34
@github-project-automation github-project-automation bot moved this from Backlog to Done in Parcels development Apr 15, 2025
@jbusecke
Copy link

@VeckoTheGecko - Do we really want to maintain a duplicate of code already provided by xgcm ? ie, why wouldn't we want to just use xgcm as a dependency ?

Oh yes, I should have mentioned this in the issue body. I think we should vendor as opposed to adding it as a dependency for the following reasons:

Also they're looking to get rid of the Axis object from the public API Lets get rid of Axis xgcm/xgcm#405 , which I think further would make things difficult for us.

* xgcm doesn't look to have active maintainers ([source](https://discourse.pangeo.io/t/status-of-xgcm/4986/5)). Their last release was in 2022 (they have an intiative for a new release though [Time to issue a new release xgcm/xgcm#670](https://github.com/xgcm/xgcm/issues/670)). The project is still stable though from what I can tell

Hey @VeckoTheGecko sorry for the late comment (we just released xgcm 0.9.0 and I saw the link). I know we chatted about this personally, but I also wanted to state this publicly.

1. xgcm is quite a specific tool (i.e., enabling grid aware calculus on ocean grids). We only want the "grid aware" part, which (a) is not particularly part of the public API, and (b) is a relatively small part of their codebase . Depending on xgcm would couple us to their development, and relying on their private API would be potentially unstable. 

I would be curious what parts of the API you are using here. I do not consider the Axis/Grid as private to be honest. We abandoned the idea of dropping axis, so I do not think that will be a risk for you folks here. I also think that the grid-awareness (and associated metadata parsing) is really the 'core product' of xgcm, whereas the calculus could at this point be entirely coupled out into one or many different packages that supply grid_ufuncs.

As you know my time is limited at the moment but xgcm is not dead. Might be worth checking out the new release too!

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.

4 participants