-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
FIX: clean up unit conversion unpacking of data, particularly for dates and pandas series #11664
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,8 @@ | |
|
||
import datetime | ||
import logging | ||
import numbers | ||
import warnings | ||
|
||
import numpy as np | ||
|
||
|
@@ -1477,9 +1479,14 @@ def have_units(self): | |
return self.converter is not None or self.units is not None | ||
|
||
def convert_units(self, x): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems to be converting anything numlike, which includes things like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great, I’ll check vs pint when I look at the other comments above. Thanks for the reminder... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I guess I still don't understand how all of this works. 😉 So |
||
# If x is already a number, doesn't need converting | ||
# If x is already a number, doesn't need converting, but | ||
# some iterables need to be massaged a bit... | ||
if munits.ConversionInterface.is_numlike(x): | ||
return x | ||
if isinstance(x, list) or isinstance(x, numbers.Number): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the rationale for returning a list as-is instead of running it through |
||
return x | ||
if issubclass(type(x), np.ma.MaskedArray): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this using |
||
return np.ma.asarray(x) | ||
return np.asarray(x) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the point of this whole addition above to short-circuit some of what would otherwise happen in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it seems worse, because calling |
||
|
||
if self.converter is None: | ||
self.converter = munits.registry.get_converter(x) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -419,6 +419,11 @@ def date2num(d): | |
return _dt64_to_ordinalf(d) | ||
if not d.size: | ||
return d | ||
if hasattr(d, 'dtype'): | ||
if ((isinstance(d, np.ndarray) and | ||
np.issubdtype(d.dtype, np.datetime64)) or | ||
isinstance(d, np.datetime64)): | ||
return _dt64_to_ordinalf(d) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What case is this whole block covering? Again, I'm confused. |
||
return _to_ordinalf_np_vectorized(d) | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -616,6 +616,16 @@ def tz_convert(*args): | |
_test_date2num_dst(pd.date_range, tz_convert) | ||
|
||
|
||
def test_dateboxplot_pandas(pd): | ||
# smoke test that this doesn't fail. | ||
data = np.random.rand(5, 2) | ||
years = pd.date_range('1/1/2000', | ||
periods=2, freq=pd.DateOffset(years=1)).year | ||
# Does not work | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another place I am confused. Is the comment obsolete? |
||
plt.figure() | ||
plt.boxplot(data, positions=years) | ||
|
||
|
||
def _test_rrulewrapper(attach_tz, get_tz): | ||
SYD = get_tz('Australia/Sydney') | ||
|
||
|
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.
I don't see any point in removing the "copy=False" kwarg. It saves time, and it is logically what is wanted here. A copy was already made via the
column_stack
call, so nothing done to coords will affect X or Y.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.
Not sure. Changed back