-
Notifications
You must be signed in to change notification settings - Fork 416
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
Improved xarray compatibility on function input and output #1353
Conversation
CartoPy 0.18 started validating scales for NaturalEarthFeature, which breaks with the scales for our own map features. There's really no reason to inherit from NaturalEarthFeature, so just make this inherit Feature and add a few missing things.
Matplotlib (as of 3.2.2) does not properly clip text to paths. For now, add a clipbox for the Axes bounding box, which will supplement the set clip path.
0.18 started autoscaling map features and updated some gridline handling, causing changes to our test images.
…ults and weird Pint __eq__ mutation issue
I've finally gotten back around to this PR! It has been brought up to date with recently implemented (and pending) changes. It is dependent on the unmerged #1378, which in turn dependent on #1372 and #1373. A preliminary review would be great at this point, if possible, but I would also understand if we need to get the dependent PRs and CI failures cleaned up first. Here's the updated status: Discussion Needed On What do we do about:
Can I defer remaining documentation updates to a follow-up PR so that this can be merged for 1.0.0rc2, and then doc updates added for 1.0.0 final? #1385 will be a big part of this. Do the two added functions ( PR Tasks Remaining
Other Notes There is a fair collection of functions that, at least as of right now, did not make sense to return xarray DataArrays even when provided them:
While I've added notes to the docstrings of all of these (where not already clear), they would probably be worth listing in the xarray & MetPy tutorial (in place of the currently supported list). Is it worth somehow documenting the rationale as to why each doesn't have xarray output? Some (like most parcel calculations) are because they take in 1D profiles and output scalars (suggesting that xarray output may be useful once gridded support is available), others (like Also, there are some functions that could see a improved API with xarray, but I'm not sure if its worth the effort at this point to implement over just have basic compatibility for now (e.g., still having to supply vertical coordinates, axis numbers, etc.):
|
Okay, high level take-aways at this point. I've run through all of the GEMPAK comparison tests (so scalar gridded output) and in general we are kosher and being able to only have to put in the main parameters is very nice! I think your listing above of what to include versus not include is reasonable. In all of the tests that I have done to date with this PR, we've hit the >95% use threshold for gridded calculations giving back an Xarray DataArray. I have a few comments on a couple of the functions, the first being the most important, the rest not solely related to this particular PR.
|
I will add that the implementation of |
Hey @tjwixtrom ! We've actually been having a lot of discussion around some of these issues and have come to a lot of those same conclusions. We've been doing some of this discussion on bi-weekly calls that I have done a really poor job of advertising more widely. So if you'd like to join in and discuss, our next one will be 1:30 MDT on 11 June on Google Meet. Happy to have more input because I really want to make sure we get the interfaces for these right--they're important. |
@dopplershift I will try and be there! |
Okay, update on Vector Calculations... Main thing is the need of broadcasting the latitudes and/or Coriolis while using DataArrays. Question: Should we normalize all of the functions (that require Coriolis) to take just latitude (and not have some take latitudes while others take Coriolis parameter) as a way of standardizing? For example, The following vector calculations have been tested: |
Found while cleaning up testing against GEMPAK calculations... When testing the Laplacian, if I feed it a DataArray, I just get back a numpy array. If I feed it a unit array, then I get unit arrays out. |
Good catch, I suspect something got messed up when I changed how the optional arguments are handled. Does the same issue arise with gradient or not? |
Yes, gradient currently has the same behavior as Laplacian, where if given a DataArray gives back just a numpy array. |
Closing since I had to re-create my MetPy fork, which means this PR no longer has a valid reference. Will put up a new PR once the new version of #1378 is complete. |
Description Of Changes
The big "xarray compatibility" PR for v1.0. This will consist of the following features:
dx
,dy
,f
,latitude
)Stretch Goal: Vectorization as many functions as possible when having DataArrays (hopefully only excluding those where 1D integration is required like parcel calculations usingmoist_lapse
)xref Adding 2D grid capabilities for SRH calculation #1258Items yet to be completed
wrap_like
argument for non-kinematics functionsbroadcast
arguments for all functions where usefulpreprocess_and_wrap
preprocess_and_wrap
doesn't work (e.g.,advection
) and add custom xarray compatibilityadvection
is the only true special case that is worth handling now with custom xarray compatiblity (others seem better deferred)e.g.,), and write new/alternative implementations to do so? (Update: most deferred, but some easy implementations were made)get_layer
and dependent functionspreprocess_and_wrap
with different combinations of parameters*_as_dataset
, all BV functions)xarray.DataArray
and not justpint.Quantity
?Supersedes #1304.
Checklist