Skip to content
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

boundary="periodic"? #195

Open
dcherian opened this issue Apr 8, 2020 · 7 comments · May be fixed by #626
Open

boundary="periodic"? #195

dcherian opened this issue Apr 8, 2020 · 7 comments · May be fixed by #626

Comments

@dcherian
Copy link
Contributor

dcherian commented Apr 8, 2020

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

@rabernat
Copy link
Contributor

rabernat commented Apr 8, 2020

This makes sense to me.

@jbusecke
Copy link
Contributor

jbusecke commented Apr 9, 2020

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

@rabernat
Copy link
Contributor

PR welcome! 🙃

@github-actions
Copy link

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!

@github-actions github-actions bot added the Stale label Jul 11, 2021
@jbusecke jbusecke added keepOpen and removed Stale labels Jul 12, 2021
@TomNicholas
Copy link
Contributor

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?

@jbusecke
Copy link
Contributor

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?

@jbusecke jbusecke added this to To do in Enabled by grid_ufunc via automation Dec 7, 2021
@TomNicholas TomNicholas added this to To do in Grid ufunc refactor via automation Dec 7, 2021
@TomNicholas TomNicholas moved this from To do to Would allow this in Grid ufunc refactor Dec 7, 2021
@raehik
Copy link
Contributor

raehik commented Aug 10, 2022

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:

xgcm/xgcm/test/test_grid.py

Lines 576 to 580 in 2b28b56

# This case is broken, need to fix!
# with pytest.raises(
# ValueError, match="`boundary=fill` is not allowed " "with periodic axis X."
# ):
# ax.interp(ds.data_c, "left", boundary="fill")

@jbusecke jbusecke linked a pull request Apr 11, 2024 that will close this issue
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Grid ufunc refactor
Would allow this
Development

Successfully merging a pull request may close this issue.

5 participants