xgcm

xgcm copied to clipboard

I keep finding it funny that xgcm has both periodic and boundary kwargs. Logically it seems like periodic should be replaced by boundary="periodic"?

This makes sense to me.

That’s a great idea, that would simplify the setup.

PR welcome! 🙃

This issue has been marked 'stale' due to lack of recent activity. If there is no further activity, the issue will be closed in another 30 days. Thank you for your contribution!

This change would make all the signatures for the functions and decorators in the grid_ufunc refactor simpler too. I don't know whether I should make this change as part of that refactor or not though?

Here is a suggestion:

Implement internal logic to understand boundary='periodic' in the refactor.

You could then implement something like this in Grid:

if periodic:
    DeprecationWarning('Dont use periodic anymore, instead set boundary='periodic')
    boundary='periodic' # this needs to handle more complex cases (e.g. defined per axis), just for illustration here.

That way none of the current tests should fail, but internally we can remove the ambiguity, and in a few versions remove periodic as input alltoghether.

Thoughts @dcherian @rabernat?

Hi, I'm taking a peek at this *the full deprecation, that is. There are lots of tests which configure the grid with both periodic, and boundary options like fill and extend. As I understand, this change should mean you can't e.g. make an axis periodic and fill at the same time. Are there any issues expected with changing this? I ask because there I think there are lots of tests this impacts.

Actually, I can see a commented-out case which would assert boundary=fill not being used together with periodic: https://github.com/xgcm/xgcm/blob/2b28b56eebfca67dbf79530db75b48e385eb6bd3/xgcm/test/test_grid.py#L576-L580