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

Making grid arguments for divergence and vorticity keyword-only as forward-looking API change #1519

Closed
jthielen opened this issue Sep 30, 2020 · 5 comments · Fixed by #1521
Closed
Labels
Area: Calc Pertains to calculations Type: API Change Changes to how existing functionality works Type: Enhancement Enhancement to existing functionality
Milestone

Comments

@jthielen
Copy link
Collaborator

jthielen commented Sep 30, 2020

It occurred to me after the overhaul of advection in #1490 that a similar "quasi-overloading" approach may work well for vorticity and divergence to handle both their 2D and 3D cases with the same function (as discussed in #684, "We can defer designs on extending divergence to 3D and vorticity to other planes until we actually can see what we're doing with xarray.").

While we definitely don't have to have the 3D forms implemented immediately for v1.0, I think it would make sense to make the grid arguments keyword-only in anticipation of this change so that we don't have an API break when adding in the optional w term. Would this be a reasonable change? Also, while we're at it, what should the behavior be for these (now-optional-with-xarray) grid arguments in the rest of the kinematics calculations (right now they're just regular optional arguments)?

@jthielen jthielen added Area: Calc Pertains to calculations Type: API Change Changes to how existing functionality works labels Sep 30, 2020
@jthielen jthielen added this to the 1.0 milestone Sep 30, 2020
@dopplershift
Copy link
Member

I think this would be a reasonable change to make. I'm not sure what you're asking about:

what should the behavior be for these (now-optional-with-xarray) grid arguments in the rest of the kinematics calculations

?

@dopplershift dopplershift added the Type: Enhancement Enhancement to existing functionality label Sep 30, 2020
@jthielen
Copy link
Collaborator Author

jthielen commented Sep 30, 2020

[...] I'm not sure what you're asking about:

what should the behavior be for these (now-optional-with-xarray) grid arguments in the rest of the kinematics calculations

?

Sorry, that was really vague. It is really just asking if there are any other kinematics functions we should make the same change to as well...i.e.: With each function, should we leave the grid arguments as they are now (as regular optional arguments, that can be specified positionaly or with keywords), or are there other cases of kinematics calculations that may have analogous desired API changes, so that we should similarly enforce the grid arguments as keyword-only?

@dopplershift
Copy link
Member

Nothing jumps to mind and I feel like we should avoid guessing. Need to avoid turning 1.0 into "get it perfect". Let's shoot for "not horribly broken".

Great project motto: "MetPy: Not horribly broken". Sadly, that's the bar for software in our field. 😉

@KCorb11
Copy link

KCorb11 commented Sep 21, 2021

Suggestion: Add absolute vorticity, so both vorticity and absolute vorticity have the same syntax with keyword arguments.

@dopplershift
Copy link
Member

@KCorb11 Thanks for the suggestion. See #2102 for an even bigger list that we plan to change when we break API compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Calc Pertains to calculations Type: API Change Changes to how existing functionality works Type: Enhancement Enhancement to existing functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants