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

ageostrophic_wind call signature #1112

Closed
dopplershift opened this issue Jul 26, 2019 · 10 comments · Fixed by #1286
Closed

ageostrophic_wind call signature #1112

dopplershift opened this issue Jul 26, 2019 · 10 comments · Fixed by #1286
Labels
Area: Calc Pertains to calculations Area: Xarray Pertains to xarray integration Type: API Change Changes to how existing functionality works Type: Maintenance Updates and clean ups (but not wrong)
Milestone

Comments

@dopplershift
Copy link
Member

The call signature of ageostrophic_wind is a bit unconventional:

ageostrophic_wind(heights, f, dx, dy, u, v, dim_order='yx')

Every other kinematic function has dx and dy as the last arguments (besides the to be removed dim_order).

This has implications for simplifying the call signature when using XArray. With the other functions, we can simply make dx and dy optional and calculate them from the coordinates when given a DataArray. Here, that won't work.

My intention is to change it to be more sensible. Current thinking:

ageostrophic_wind(heights, u, v, f=None, dx=None, dy=None)

(f should be calculable from the coordinates as well.) So how do we give a migration path on this? Just warn in the 0.11 and 0.12 releases and break in 1.0? I'm open to suggestions.

@dopplershift dopplershift added Area: Calc Pertains to calculations Area: Xarray Pertains to xarray integration Type: API Change Changes to how existing functionality works Type: Maintenance Updates and clean ups (but not wrong) labels Jul 26, 2019
@dopplershift dopplershift added this to the 0.11 milestone Jul 26, 2019
@zbruick
Copy link
Contributor

zbruick commented Jul 26, 2019

Seems to me like your suggested order of variables makes the most sense and fits with the rest of the API better. As far as converting it, I think your suggestion to warn for the next two releases and incorporate it into the rest of the API changes for 1.0 make the most sense.

@zbruick
Copy link
Contributor

zbruick commented Aug 16, 2019

Revisiting this now - by just warn for now, do you mean that we don't have any code change for now, and just provide a warning whenever this function is called? I imagine we can't actually update the order of the variables now as there isn't a way to actually deprecate that?

@dopplershift
Copy link
Member Author

I think that's right. Can use a FutureWarning with warnings.warn.

@zbruick
Copy link
Contributor

zbruick commented Aug 16, 2019

Ok that makes sense. I'll get something up on this shortly, and add it to the 1.0 deprecation list.

@akrherz
Copy link
Contributor

akrherz commented Aug 16, 2019

About the only API suggestion I can make is to convert this definition to ageostrophic_wind(*args, **kwargs) and then emit the warning when 5 positional arguments are passed suggesting that they instead pass in 3 positional and kwargs for the rest. Then in two releases, drop that logic. Then there is actionable things the end-user can do? shrug.

@dopplershift
Copy link
Member Author

I think I hear what you're saying: right now, we're talking about telling users that they'll need to do something in 4 months, but giving them nothing they can do today, and no way of making the warning go away. Let me see how I can do better...

@akrherz
Copy link
Contributor

akrherz commented Aug 16, 2019

Thanks, what I would immediately do is add warnings.simplefilter("ignore", FutureWarning) to the top of each script and then forget about this and get burned anyway when 1.0 comes out. :)

@dopplershift
Copy link
Member Author

@akrherz So one option that comes to mind is simply having a version in a parallel module:

from metpy.future import ageostrophic_wind
...
ageostrophic_wind(h, u, v, f, dx, dy)

At some point we could warn, then drop ageostrophic_wind from metpy.future.

We could go further and try to mimic Python's __future__, but that's going to require quite a bit of voodoo that I'm not convinced is worth it. What do you think about my proposed solution?

@akrherz
Copy link
Contributor

akrherz commented Aug 16, 2019

that seems good to me, thanks for the care taken here.

@dopplershift dopplershift modified the milestones: 0.11, 1.0 Aug 20, 2019
@dopplershift
Copy link
Member Author

Moved this to 1.0 to capture the need to make the "future" the present. Otherwise, the future module has been implemented in master for 0.11.

dopplershift added a commit to dopplershift/MetPy that referenced this issue Jan 10, 2020
…ta#1112)

This makes their behavior standard and eliminates the old behavior.
dopplershift added a commit to dopplershift/MetPy that referenced this issue Jan 11, 2020
…ta#1112)

This makes their behavior standard and eliminates the old behavior.
dopplershift added a commit to dopplershift/MetPy that referenced this issue Jan 11, 2020
…ta#1112)

This makes their behavior standard and eliminates the old behavior.
dopplershift added a commit to dopplershift/MetPy that referenced this issue Jan 11, 2020
…ta#1112)

This makes their behavior standard and eliminates the old behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Calc Pertains to calculations Area: Xarray Pertains to xarray integration Type: API Change Changes to how existing functionality works Type: Maintenance Updates and clean ups (but not wrong)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants