-
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
Derivatives & map factors #2743
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.
I did a quick check with this PR on the vorticity calculation and it checks out exactly (except for the boundary) for projected coords, but there still remains a very small difference (error on order 0.01) between the GEMPAK calculation and this map projection aware vorticity calculation for a lat/lon grid. It appears that the difference is pent up in the default latlong projection in PyProj uses the WGS84 ellipse where GEMPAK uses generic 'sphere'. If you create the CRS using `CRS(proj='latlong', ellps='sphere') you can nearly perfectly recreate the GEMPAK calculation. I don't know that one (using the generic sphere vs WGS84) is more correct than another as projecting a three dimensional sphere on to a 2D plane has many issues/trade offs. I would be comfortable moving ahead with the vector derivatives as done in this draft PR. |
10307bb
to
f052ea2
Compare
@kgoebber I can cut that to ~0.0041 in your test notebook: gfs_data = xr.open_dataset("data/gfs_test_data.nc").metpy.assign_crs(
{"grid_mapping_name": "latitude_longitude",
"earth_radius": 6371229
}
) That's the value (now) assigned by the TDS decoding the GRIB. It's also close to a value I found in some GEMPAK projection code of 6371221.3. I think PROJ's default of WGS84 is fine with missing metadata. Eventually we may want to try to be smarter and fill in for known grids (ala #1289), but I think our math here is sound. Full details:
|
@kgoebber whenever you get a chance, can you drop us any new comparisons you've put together? We're testing some functions today and more sanity checks are always better |
Thanks for the poke.
I've sent the notebook file to compare vorticity, absolute vorticity, divergence, three deformations, potential vorticity, and frontogenesis. The graphics for potential vorticity and frontogenesis are not great because their scales don’t conform as well to all of the other calculations.
I was playing a bit with this end of last week and with only the small modifications was able to get divergence and the deformation calculations to correspond well. I was having more difficulty in getting potential vorticity and frontogensis to improve much.
|
Okay, so I was able to do a bit more digging on frontogenesis and it appears that the new decorator (parse_grid_arguments) is diving different dx and dy values from what were being given by the old decorator (add_grid_arguments_from_xarray). So it is giving the correct information for divergence, shearing, stretching, and total deformation, but not for the other derivatives. |
""" | ||
# u, v, *, dx=None, dy=None, x_dim=-1, y_dim=-2, longitude=None, latitude=None, crs=None, | ||
# parallel_scale=None, meridional_scale=None, return_only=None | ||
from ..xarray import dataarray_arguments |
Check notice
Code scanning / CodeQL
Cyclic import
Current todo list, mainly to capture context before I go off for the Thanksgiving holiday and forget all of this:
|
After further testing, the issues that I was having with For example, I added the scale factors as follows within the # Get gradients of potential temperature in both x and y
ddy_theta = meridional_scale * first_derivative(potential_temperature, delta=dy,
axis=y_dim)
ddx_theta = parallel_scale * first_derivative(potential_temperature, delta=dx,
axis=x_dim) Comparing the calculations of gradient on NAM data, again, if not using lat/lon values to do derivatives, the So what does all of this mean for our implementation? I think we should go with a method for all calculations to NOT use lat/lon and instead bake in |
Ok, I've updated absolute vorticity and adjusted the decorator infrastructure to account for its new needs (we were dynamically injecting latitude, etc. into the function signature, but it already had it and needed it passed to it). This resulted also in a few places using |
24e5d8f
to
f905901
Compare
When I run absolute vorticity against by comparison tests, I get a broadcast error. The latitude variable is still single dimensional. |
@kgoebber Good catch. I've fixed (we were using lat/lon as unit arrays rather than DataArrays prematurely) and added a test. Interestingly, the test didn't crash (using our 4D data), but had incorrect values due to lat and lon both having the same size. |
07e193c
to
24f5655
Compare
Absolute vorticity checks out now for GFS. I'm running into problems with projected datasets (e.g., NAM, GFS 104 grid) that have two-dimensional index (e.g., x/y or lat/lon variables). Here is the error: ---------------------------------------------------------------------------
ValueError Traceback (most recent call last)
Cell In [5], line 10
7 data_var_u = nam_data['u-component_of_wind_isobaric'].metpy.sel(time=datetime(2018, 3, 8, 0), vertical=500*units.hPa).metpy.quantify()
8 data_var_v = nam_data['v-component_of_wind_isobaric'].metpy.sel(time=datetime(2018, 3, 8, 0), vertical=500*units.hPa).metpy.quantify()
---> 10 vor = mpcalc.vorticity(data_var_u, data_var_v)
12 print('Vorticity \n')
13 error_stats(vor[3:-3,3:-3]*1e5, out_var[3:-3,3:-3]*1e5)
File ~/unidata_stuff/MetPy/src/metpy/calc/tools.py:1162, in parse_grid_arguments.<locals>.wrapper(*args, **kwargs)
1160 if grid_prototype is not None:
1161 coords = list(grid_prototype.metpy.coordinates('latitude', 'longitude'))
-> 1162 p_scale = xr.DataArray(p_scale,
1163 coords=coords).broadcast_like(grid_prototype)
1164 m_scale = xr.DataArray(m_scale,
1165 coords=coords).broadcast_like(grid_prototype)
1167 bound_args.arguments['parallel_scale'] = p_scale
File ~/miniconda3/envs/devel/lib/python3.10/site-packages/xarray/core/dataarray.py:412, in DataArray.__init__(self, data, coords, dims, name, attrs, indexes, fastpath)
410 data = _check_data_shape(data, coords, dims)
411 data = as_compatible_data(data)
--> 412 coords, dims = _infer_coords_and_dims(data.shape, coords, dims)
413 variable = Variable(dims, data, attrs, fastpath=True)
414 indexes, coords = _create_indexes_from_coords(coords)
File ~/miniconda3/envs/devel/lib/python3.10/site-packages/xarray/core/dataarray.py:125, in _infer_coords_and_dims(shape, coords, dims)
123 else:
124 for n, (dim, coord) in enumerate(zip(dims, coords)):
--> 125 coord = as_variable(coord, name=dims[n]).to_index_variable()
126 dims[n] = coord.name
127 dims = tuple(dims)
File ~/miniconda3/envs/devel/lib/python3.10/site-packages/xarray/core/variable.py:543, in Variable.to_index_variable(self)
541 def to_index_variable(self):
542 """Return this variable as an xarray.IndexVariable"""
--> 543 return IndexVariable(
544 self.dims, self._data, self._attrs, encoding=self._encoding, fastpath=True
545 )
File ~/miniconda3/envs/devel/lib/python3.10/site-packages/xarray/core/variable.py:2720, in IndexVariable.__init__(self, dims, data, attrs, encoding, fastpath)
2718 super().__init__(dims, data, attrs, encoding, fastpath)
2719 if self.ndim != 1:
-> 2720 raise ValueError(f"{type(self).__name__} objects must be 1-dimensional")
2722 # Unlike in Variable, always eagerly load values into memory
2723 if not isinstance(self._data, PandasIndexingAdapter):
ValueError: IndexVariable objects must be 1-dimensional Something in xarray when we set the coordinate values for the parallel and meridional scale DataArrays. |
I can see why this doesn't work when lat/lon aren't the native coords (we're not creating the coords = list(grid_prototype.metpy.coordinates('latitude', 'longitude'))
p_scale = xr.DataArray(p_scale, coords=coords).broadcast_like(grid_prototype) Now the question is how to test that case... |
Comparisons are looking good for GFS at this point including frontogenesis and potential vorticity. There is more of a difference for potential vorticity (correlation is 0.993, which is on the lowest end for our comparisons), but feel that is likely due to vertical differencing. |
It's been completely replaced by parse_grid_arguments, and is only used by a single test. Move that to be a test of parse_grid_arguments.
The case with the dataarray without a CRS especially revealed some issues with the logic that needed addressing.
In this case it should default to assuming (..., Y, X) ordering.
Support previous use-case of vertical-only advection and xarray with parsed dims by escaping before scale calculations.
f61800c
to
f963218
Compare
This is a draft to expand @jthielen's implementation (drafted separately #893) for derivatives with support for map factors and the associated machinery to enable calculating (or providing) those map factors. These should be the minimum commits needed to test vorticity calculations on most well-described xarray dataarrays. We will be using this draft for expanding tests/validation and covered functions, unbreaking the preprocessing, and cleaning up the implementation for inclusion in 1.4 this week.