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

coriolis_parameter: output error corrected. #1464

Closed
wants to merge 2 commits into from

Conversation

livialmg
Copy link

The former coriolis_parameter function throws an error: "AttributeError: 'Variable' object has no attribute 'to'"
With a minor change, it was corrected.

The former 'coriolis_parameter' function throws an error: "Variable object has no attribute to".
With a minor change, this error was corrected.
@CLAassistant
Copy link

CLAassistant commented Aug 14, 2020

CLA assistant check
All committers have signed the CLA.

@dopplershift
Copy link
Member

Thanks for the contribution. It looks like something went wrong, though. There's an extra basic.py added here in the base of the repository. Also, the version of src/metpy/calc/basic.py looks out of date (it's restoring some old parameter names). Can you update and fix?

@dopplershift dopplershift added Area: Calc Pertains to calculations Type: Bug Something is not working like it should labels Aug 14, 2020
@lgtm-com
Copy link

lgtm-com bot commented Aug 14, 2020

This pull request introduces 2 alerts when merging 285b474 into 51a4172 - view on LGTM.com

new alerts:

  • 2 for Unused import

@@ -585,7 +581,8 @@ def coriolis_parameter(latitude):

"""
latitude = _check_radians(latitude, max_radians=np.pi / 2)
return (2. * mpconsts.omega * np.sin(latitude)).to('1/s')
f = 2. * mpconsts.omega * np.sin(latitude)
return f.data.to('1/sec')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This unfortunately looks like it would be a mutually conflicting change with the upcoming xarray updates for 1.0 (#1353). Would you be willing to write a test case or short example of the error you had encountered, so that I can verify that it is resolved in the updated version of #1353? Thank you!

Copy link
Author

@livialmg livialmg Aug 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

`import numpy as np
from metpy import calc as mpcalc
import xarray as xr

file_in = 'test.nc'

file = xr.open_dataset(file_in)
lat = file.variables['lat'][:]
f = mpcalc.coriolis_parameter(np.deg2rad(lat))`

I wrote this small piece of code to demonstrate the error I'm getting.
test.nc is a sample from my actual data:

------------------------------------------------------------------------
dimensions:
	lon = 321 ;
	lat = 281 ;
variables:
	double lon(lon) ;
		lon:units = "degrees_east" ;
		lon:long_name = "Longitude" ;
	double lat(lat) ;
		lat:units = "degrees_north" ;
		lat:long_name = "Latitude" ;
	double temp(lat, lon) ;
		temp:_FillValue = -999000000. ;
}
---------------------------------------------------------------------

The error message is:
"File "/home/livia/MetPy/src/metpy/xarray.py", line 1067, in wrapper
return func(*args, **kwargs)

File "/home/livia/MetPy/src/metpy/calc/basic.py", line 588, in coriolis_parameter
return (2. * mpconsts.omega * np.sin(latitude)).to('1/s')

AttributeError: 'Variable' object has no attribute 'to'"

When chaging the line 588 in basic.py, from:

return (2. * mpconsts.omega * np.sin(latitude)).to('1/s')

to

f = (2. * mpconsts.omega * np.sin(latitude))
return f.data.to('1/s')

then, I get an array if the coriolis parameter.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, it looks like you've found a corner-case in our existing xarray handling! xarray Variable objects are low-level objects that are not meant to be used on the user level, and so, we never built handling for them explicitly. This would be a good thing to document in MetPy's xarray guide as well.

I would recommend the following to fix your issue:

file = xr.open_dataset(file_in)
lat = file['lat']
f = mpcalc.coriolis_parameter(lat)

This will allow MetPy's handling of DataArrays to be used, with units automatically handled. Also, unfortunately while your proposed fix makes the calculation work for Variables, it breaks it for the standard case of pint.Quantity.

Hopefully this resolves your issue, and we can close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Calc Pertains to calculations Type: Bug Something is not working like it should
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants