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.

Sign up for GitHub

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

Jump to bottom

Add gradient and derivatives for irregular grids #692

Merged

dopplershift merged 13 commits into Unidata:master from dopplershift:gradient

Jan 4, 2018

Member

dopplershift commented Dec 18, 2017 •

edited

Loading

This should be good to go now. This adds 1st and 2nd derivatives, as well as gradient and laplacian that work with irregular grids. This also updates the kinematics functions to use them rather than wrappers for numpy's gradient function.

Closes #174 .

dopplershift requested a review from jrleeman as a code owner

December 18, 2017 04:27

stickler-ci reviewed

View reviewed changes

metpy/calc/tools.py Outdated



		def first_derivative():
		a = 'foobar'

stickler-ci Dec 18, 2017

F841 local variable 'a' is assigned to but never used

metpy/calc/tools.py Outdated

+              def first_derivative():
+                  a = 'foobar'
+                  return b

stickler-ci Dec 18, 2017

F821 undefined name 'b'
W292 no newline at end of file

dopplershift added 3 commits

January 2, 2018 14:51


          MNT: Remove hyphen from reference tag

b4e09a2

It doesn't break Sphinx, but it does break PyCharm. Just get rid of it
since it's not necessary.


          MNT: Update matplotlib import in conftest

For some reason pyplot needs to be imported explicitly. Easy enough just
to fix.


          ENH: Make unit versions of at_least1d/atleast_2d handle arrays

fdac87e

They were assuming they always got Quantities.

dopplershift force-pushed the gradient branch from c33e0ec to 85e9b4c Compare

January 3, 2018 08:21

stickler-ci reviewed

View reviewed changes

metpy/calc/tests/test_tools.py Outdated

+                  """Test lapacian with full 2D arrays."""
+                  laplac_true = 2 * (np.ones_like(deriv_2d_data.f) * (deriv_2d_data.a + deriv_2d_data.b))
+                  laplac = laplacian(deriv_2d_data.f, x=(deriv_2d_data.y, deriv_2d_data.x))
+                  assert_array_almost_equal(laplac, laplac_true , 5)

stickler-ci Jan 3, 2018

E203 whitespace before ','

metpy/calc/tools.py Outdated

+                  combined_delta = delta[delta_slice0] + delta[delta_slice1]
+                  delta_diff = delta[delta_slice1] - delta[delta_slice0]
+                  center = (- delta[delta_slice1] / (combined_delta * delta[delta_slice0]) * f[slice0]
+                            + delta_diff / (delta[delta_slice0] * delta[delta_slice1]) * f[slice1]

stickler-ci Jan 3, 2018

W503 line break before binary operator

metpy/calc/tools.py Outdated

+                  delta_diff = delta[delta_slice1] - delta[delta_slice0]
+                  center = (- delta[delta_slice1] / (combined_delta * delta[delta_slice0]) * f[slice0]
+                            + delta_diff / (delta[delta_slice0] * delta[delta_slice1]) * f[slice1]
+                            + delta[delta_slice0] / (combined_delta * delta[delta_slice1]) * f[slice2])

stickler-ci Jan 3, 2018

W503 line break before binary operator

metpy/calc/tools.py Outdated

+                  combined_delta = delta[delta_slice0] + delta[delta_slice1]
+                  big_delta = combined_delta + delta[delta_slice0]
+                  left = (- big_delta / (combined_delta * delta[delta_slice0]) * f[slice0]
+                          + combined_delta / (delta[delta_slice0] * delta[delta_slice1]) * f[slice1]

stickler-ci Jan 3, 2018

W503 line break before binary operator

metpy/calc/tools.py Outdated

+                  big_delta = combined_delta + delta[delta_slice0]
+                  left = (- big_delta / (combined_delta * delta[delta_slice0]) * f[slice0]
+                          + combined_delta / (delta[delta_slice0] * delta[delta_slice1]) * f[slice1]
+                          - delta[delta_slice0] / (combined_delta * delta[delta_slice1]) * f[slice2])

stickler-ci Jan 3, 2018

W503 line break before binary operator

metpy/calc/tools.py Outdated

+                  combined_delta = delta[delta_slice0] + delta[delta_slice1]
+                  center = 2 * (f[slice0] / (combined_delta * delta[delta_slice0])
+                                - f[slice1] / (delta[delta_slice0] * delta[delta_slice1])
+                                + f[slice2] / (combined_delta * delta[delta_slice1]))

stickler-ci Jan 3, 2018

W503 line break before binary operator

metpy/calc/tools.py Outdated

+                  combined_delta = delta[delta_slice0] + delta[delta_slice1]
+                  left = 2 * (f[slice0] / (combined_delta * delta[delta_slice0])
+                              - f[slice1] / (delta[delta_slice0] * delta[delta_slice1])

stickler-ci Jan 3, 2018

W503 line break before binary operator

metpy/calc/tools.py Outdated

+                  combined_delta = delta[delta_slice0] + delta[delta_slice1]
+                  left = 2 * (f[slice0] / (combined_delta * delta[delta_slice0])
+                              - f[slice1] / (delta[delta_slice0] * delta[delta_slice1])
+                              + f[slice2] / (combined_delta * delta[delta_slice1]))

stickler-ci Jan 3, 2018

W503 line break before binary operator

metpy/calc/tools.py Outdated

+                  combined_delta = delta[delta_slice0] + delta[delta_slice1]
+                  right = 2 * (f[slice0] / (combined_delta * delta[delta_slice0])
+                               - f[slice1] / (delta[delta_slice0] * delta[delta_slice1])

stickler-ci Jan 3, 2018

W503 line break before binary operator

metpy/calc/tools.py Outdated

+                  combined_delta = delta[delta_slice0] + delta[delta_slice1]
+                  right = 2 * (f[slice0] / (combined_delta * delta[delta_slice0])
+                               - f[slice1] / (delta[delta_slice0] * delta[delta_slice1])
+                               + f[slice2] / (combined_delta * delta[delta_slice1]))

stickler-ci Jan 3, 2018

W503 line break before binary operator

dopplershift changed the title ~~WIP: Add gradient~~ Add gradient and derivatives for irregular grids

dopplershift added this to the 0.7 milestone

dopplershift force-pushed the gradient branch 2 times, most recently from 16c5524 to 7a6b93f Compare

January 3, 2018 08:53

Member Author

dopplershift commented Jan 3, 2018

Need to find some way to make Code Climate less noisy--but I think we can ignore it here.

jrleeman suggested changes

View reviewed changes

Contributor

jrleeman left a comment

A few things that should probably be at least thought about - overall a huge improvement to our functionality!

metpy/units.py

-                      return units.Quantity(ret, orig_units[0])
-                  return [units.Quantity(m, u) for m, u in zip(ret, orig_units)]
+                      if orig_units[0] is not None:
+                          return units.Quantity(ret, orig_units[0])

Contributor

jrleeman Jan 3, 2018

Given that we just saw some weirdness in attaching units this way in #619 - maybe go with ret * orig_units[0]

Member Author

dopplershift Jan 3, 2018 •

edited

Loading

Given that I don't understand what went wrong on that one, I'm not inclined to fix it proactively. I'd much prefer to have an actual test case to validate that the new way would be the right one. Either way outside the scope of this PR.

metpy/units.py

+                          return units.Quantity(ret, orig_units[0])
+                      else:
+                          return ret
+                  return [units.Quantity(m, u) if u is not None else m for m, u in zip(ret, orig_units)]

Contributor

jrleeman Jan 3, 2018

Same

Member Author

dopplershift Jan 3, 2018

see above

metpy/units.py

-                      return units.Quantity(ret, orig_units[0])
-                  return [units.Quantity(m, u) for m, u in zip(ret, orig_units)]
+                      if orig_units[0] is not None:
+                          return units.Quantity(ret, orig_units[0])

Contributor

jrleeman Jan 3, 2018

same

Member Author

dopplershift Jan 3, 2018

again

metpy/units.py

+                          return units.Quantity(ret, orig_units[0])
+                      else:
+                          return ret
+                  return [units.Quantity(m, u) if u is not None else m for m, u in zip(ret, orig_units)]

Contributor

jrleeman Jan 3, 2018

same

Member Author

dopplershift Jan 3, 2018

again

metpy/calc/tools.py

+                  n, axis, delta = _process_deriv_args(f, kwargs)
+                  # create slice objects --- initially all are [:, :, ..., :]
+                  slice0 = [slice(None)] * n

Contributor

jrleeman Jan 3, 2018

Given that these 10-ish lines are duplicated in the first derivative, should they be in a helper somewhere? Trivial lines, but more to maintain/bugfix and makes the functions even longer.

Member Author

dopplershift Jan 3, 2018

I thought about this, but they each have their own set of 5 slices that would have to be passed in--that seemed like it wouldn't simplify enough to justify.

metpy/calc/tools.py



		def _process_gradient_args(kwargs):
		"""Handle common processing of arguments for gradient and gradient-like functions."""

Contributor

jrleeman Jan 3, 2018

Should there be an exception raised when a user through accident or mis-understanding passes both instead of just grabbing deltas and ignoring x silently?

Member Author

dopplershift Jan 3, 2018

Fair enough.

metpy/calc/tools.py

+                  if f.shape[axis] < 3:
+                      raise ValueError('f must have at least 3 point along the desired axis.')
+                  if 'delta' in kwargs:

Contributor

jrleeman Jan 3, 2018

Similar question here on silently ignoring an argument if both are passed.

Member Author

dopplershift Jan 3, 2018

ok

metpy/calc/tools.py Outdated

		return n, axis, delta


		def _diff(x, **kwargs):

Contributor

jrleeman Jan 3, 2018

Should this be in units like atleast_1d and be more publicly accessible? I could see it being handy.

Member Author

dopplershift Jan 3, 2018

Meh, ok.

jrleeman reviewed

View reviewed changes

metpy/calc/kinematics.py

                   return dudx - dvdy
               @exporter.export
+              @deprecated('0.7', addendum=' Use stretching_deformation and/or shearing_deformation instead.',

Contributor

jrleeman Jan 3, 2018

Should also probably note in the docs like we do elsewhere:

    .. deprecated:: 0.6.0
      Function has been moved to the Siphon library and will be removed from MetPy in 0.8.0.

Member Author

dopplershift Jan 3, 2018

Ah, I missed that we did that. Will fix.

jrleeman mentioned this pull request

Add high contrast functionality to timestamp #697

Merged

Contributor

jrleeman commented Jan 3, 2018

Also, deprecations will need to be added to #689 or #690 as appropriate.

dopplershift added 8 commits

January 3, 2018 15:24


          ENH: Add functions for derivatives on irregular grids

43806e7

Adds first and second derivatives, as well as gradient and laplacian
functions. These work on grids that have uneven spacing between points.


          BUG: Fix incorrect deprecation

b36f782

The original, actually shipped name was `h_convergence`, not
`convergence`.


          MNT: Elminate divergence_vorticity

09ca99d

With the new derivative functions, there's no performance benefit to the
combined function. Instead of adding the appropriate named version, just
deprecate the old one entirely in favor of the individual vorticity and
divergence functions.


          MNT: Deprecate shearing_stretching_deformation

8bf6020

This will no longer give us any performance benefit, so remove it.


          MNT: Silence deprecation warnings in tests

fade954


          MNT: Fix typos

be27c60


          ENH: Replace use of numpy.gradient wrapper

35d1824

Use our own first_derivative and gradient functions. Some tests needed
to be updated to account for changes to how values at the edge of the
grid are treated.


          MNT: Disable printing test log output by default

2e32e2c

dopplershift force-pushed the gradient branch from 7a6b93f to 092f69a Compare

January 3, 2018 22:24

stickler-ci reviewed

View reviewed changes

metpy/calc/tools.py Outdated

+                          delta = _broadcast_to_axis(delta, axis, n)
+                  elif 'x' in kwargs:
+                      x = _broadcast_to_axis(kwargs['x'], axis, n)
+                      delta = _diff(x, axis=axis)

stickler-ci Jan 3, 2018

F821 undefined name '_diff'

metpy/calc/tools.py Outdated

		raise ValueError('Must specify either "x" or "delta" for value positions.')

		return n, axis, delta

stickler-ci Jan 3, 2018

W391 blank line at end of file


          ENH: Promote diff wrapper to part of the API

379a8bf

dopplershift force-pushed the gradient branch from 092f69a to 379a8bf Compare

January 3, 2018 22:25

Member Author

dopplershift commented Jan 3, 2018

Other issues are already updated for deprecations.

jrleeman previously approved these changes

View reviewed changes

Contributor

jrleeman left a comment

Looks good!

dopplershift added Area: Calc Type: Enhancement labels

dopplershift dismissed jrleeman’s stale review via

d79c96f

January 3, 2018 23:10

jrleeman previously approved these changes

View reviewed changes

dopplershift dismissed jrleeman’s stale review via

26f718a

January 3, 2018 23:28

dopplershift force-pushed the gradient branch from d79c96f to 26f718a Compare

January 3, 2018 23:28

jrleeman previously approved these changes

View reviewed changes


          MNT: Handle normalize_axis_index for old numpy

f73d4b8

It's just a nice helper for better error messages, so we can just
backport as a noop.

dopplershift dismissed jrleeman’s stale review via

f73d4b8

January 3, 2018 23:41

dopplershift force-pushed the gradient branch from 26f718a to f73d4b8 Compare

January 3, 2018 23:41

jrleeman approved these changes

View reviewed changes

dopplershift merged commit f477fdc into Unidata:master

dopplershift deleted the gradient branch

January 4, 2018 00:36

eric-wieser commented Jan 4, 2018

It'd be great to get something like this into numpy

eric-wieser reviewed

View reviewed changes

metpy/calc/tools.py

               import functools
               import warnings
               import numpy as np
+              try:
+                  from numpy.core.numeric import normalize_axis_index

eric-wieser Jan 4, 2018

Should really be np.core.multiarray instead

Member Author

dopplershift Jan 4, 2018

That makes more sense, good to know. I'll look at a PR to numpy some time when I have some cycles...

Not sure what I'd call the damned thing 😁

eric-wieser Jan 4, 2018

Refactoring np.gradient to use your _first_derivative would be a good first step

Member Author

dopplershift Jan 4, 2018

There's a pretty steep performance penalty (or it would seem from the complexity) for the support for irregular spacing though...

eric-wieser Jan 4, 2018

We already have irregular spacing - all we don't currently support is broadcasting of dx against the values.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: Calc Type: Enhancement