Improved xarray compatibility on function input and output

[jthielen]

Overview Of Changes

At long last, the updated version of #1353. This PR (and hopefully not yet another replacement) is the big "xarray compatibility" PR for v1.0. Feature highlights:

• Ability to automatically broadcast input xarray arguments
• Wrapping function output as a DataArray with the coordinates of one of the inputs
• Nearly all array-returning functions in library returning DataArrays when provided DataArrays
• Automatic inclusion of grid parameters for kinematics calculations (e.g., dx, dy, f, latitude)
• Dimension order independence (through specifying x_dim and y_dim kwargs or automatic identification from DataArrays)
• Minimal handling for xarray Variables (xref coriolis_parameter: output error corrected. #1464)
• Overhauled advection function (simplified implementation and greater consistency with the rest of kinematics; xref Inconsistent arguments for gradient and advection #896)
• Changes axis argument to vertical_dim for consistency when referring uniquely to vertical dimension (for consistency with kinematics)

Remaining To-Do List/Discussion Items

I wanted to get this PR up as soon as I had it in a sufficient state for first review and not wait too long, since it has already taken far to long to get to this point! Below are remaining to-dos and unresolved discussion items from before:

To-Dos:

• Fix-up broken old tests (axis arg, advection)
• Add tests for
  • parcel_profile_with_lcl_as_dataset (new)
  • isentropic_interpolation_as_dataset (new)
  • advection (mind as well be new)
  • new decorators (preprocess and wrap, add grid arguements, add vertical dim)
  • example scalar thermo calculation with xarray
  • example parcel calculation with xarray
  • example kinematics calculation with xarray (probably best to take care of Use PyProj instead of Cartopy for internal coordinate transforms and rename crs coordinate #1483 first in case values change)
• Take needed actions on discussion items below

Discussion Items:

• Does the overhaul of advection seem reasonable? A: Yes
• Were we happy with the *_as_dataset additions, or should those become the primary APIs, and the originals renamed/made private? A: Good with them as is
• Kinematics calculations somewhat inconsistently take latitude and f. Should we standardize around one or the other, or at least document why each function has each (such as not standardizing around latitude to allow f to be specified for f or beta plane approximated calculations)? @kgoebber A: use latitude
• What should we do about turbulence calculations? (@tjwixtrom, TKE Calculation on Resampled Timeseries  #1384) Might be bit too much of a haul to still have ready for 1.0, but also if we are going to change APIs, now is when to do it. A: Punt
• What should we do about interpolation calculations? (still likely quite useful for xarray-type data, but current API is not xarray-friendly, and might not be feasible for 1.0) A: Punt
• What do we do about a few odd calculations:
  • lifted_index? (API doesn't make as much sense in context of xarray, but likely still useful) A: Use as-is
  • angle_to_direction? (the one function where output in DataArray may be desired, but can't use Quantities as intermediary because the output is strings, rather than numeric) A: Use as-is

Checklist

• Closes Add standard data object #10, closes Inconsistent arguments for gradient and advection #896, closes coriolis_parameter: output error corrected. #1464
• Tests added (For added functionality and minimal wrapper usage tests, full testing of all applications of wrapper deferred to Systematize calculation testing #1214 or a follow-up)
• Fully documented (Deferred to post RC2 PR: 1.0 Upgrade guide #1385)

[kgoebber]

Quick review of a couple of items. For the most part, everything seems to be working well from my end when using my GEMPAK comparison notebook that I used to test an earlier implementation. I've noted a couple of things in various parts, mainly around advection, which has a minus sign error and the adding of dx and dy is not working as I think it is intended. Additionally, the broadcasting of f and latitude also doesn't appear to be working.

I also need to investigate the inertial-advective wind a bit more, there are some vectors that are not as closely aligned with what comes out of GEMPAK...but that will have to wait and really unrelated to this PR.

[jthielen]

Working on this now, particularly updating geostrophic_wind and ageostrophic_wind to use latitude instead of f as an argument, and was horrified to find this:

MetPy/tests/calc/test_kinematics.py

Lines 205 to 207 in 2c2d442

	# Using g as the value for f allows it to cancel out
	ug, vg = geostrophic_wind(z.T, g.magnitude / units.sec,
	100. * units.meter, 100. * units.meter)

Turns out the git blame for using an f value several orders of magnitude greater than that possible on Earth goes back to the recesses of MetPy history in 2009. So it looks like we'll be getting essentially new tests for geostrophic/ageostrophic wind!

[ghost]

DeepCode's analysis on #98600d found:

• ⚠️ 1 warning, ℹ️ 1 minor issue. 👇

Top issues

Description	Example fixes
The call to next should be guarded with a try/except block Occurrences: xarray.py:1097	🔧 Example fixes
All elements of this list point to the same object. Modifying any element of the list will result in modifying all of them. Consider using a comprehension that will create a new object for every element. Occurrences: test_calc_tools.py:1093 test_calc_tools.py:1112 test_calc_tools.py:1116	🔧 Example fixes

👉 View analysis in DeepCode’s Dashboard | Configure the bot

[dopplershift]

Hey, it made sense as a simple way to make sure the function worked as expected...but yes, not a very generalizable test. Oops. I'm actually amazed tests from my grad school days survived.

[jthielen]

This should now be ready for a full review! All tests and docs pass for me locally, so we'll see what CI shows.

(@kgoebber I found that the latitude issue was that my grid argument decorator was returning latitude as a Quantity and not a DataArray, so it wasn't being broadcast via the xarray decorator. That should be fixed now.)

[kgoebber]

Using the GEMPAK comparison notebooks that I have, all of those calculations checkout and it is much cleaner being able to use DataArrays as input and have various other parameters computed and input in the background.

Broadcasting of latitude also works for all instances checked and advection now has the correct sign.

[dopplershift]

This looks really good and is just about there.

[dopplershift]

@jthielen If you don't have the time to make any needed changes, it would be helpful to have your responses and I can make changes.

[jthielen]

I've made the direct changes based on feedback. I should be able to fix the remaining items once you get the chance to reply to my responses @dopplershift.

Thanks for the reviews!

[dopplershift]

We can ignore CodeClimate here. The coverage aren't too great, and stem from a bunch of warnings/exceptions that aren't hit.

[jthielen]

[...] The coverage aren't too great, and stem from a bunch of warnings/exceptions that aren't hit.

Would you want me to go back and add tests for those (could probably do that this evening)? Or just merge and not hold this up any longer?

[dopplershift]

Or just merge and not hold this up any longer?

Yeah, this.

[dcamron]

Apologies for the wait, but I feel like I've got a pretty good grapple on everything going in and no big hold-ups from this point on my end! Super appreciative of this and excited to see it go in. Onward to #1385!