-
Notifications
You must be signed in to change notification settings - Fork 224
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
Allow datetime inputs to region argument #562
Conversation
Includes: - Python datetime objects - np.datetime64 - pandas.Timestamp - xarray.DataArray with datetime64 dtype
except AssertionError: | ||
# convert datetime-like items to string format | ||
value[index] = np.datetime_as_string( | ||
np.asarray(item, dtype=np.datetime64) |
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.
We use np.datetime_as_string(array_to_datetime(vector))
to convert datetime arrays to string arrays.
Lines 778 to 782 in 2ddbb0e
if gmt_type == self["GMT_DATETIME"]: | |
vector_pointer = (ctp.c_char_p * len(vector))() | |
vector_pointer[:] = np.char.encode( | |
np.datetime_as_string(array_to_datetime(vector)) | |
) |
The
array_to_datetime
function uses pd.to_datetime(array)
pygmt/pygmt/clib/conversion.py
Line 324 in 2ddbb0e
return pd.to_datetime(array) |
Any specific reason to use np.asarray
here?
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.
Main reason is to handle xarray.DataArray
inputs. The array_to_datetime
/pd.to_datetime
function is meant to handle arrays/lists, but not scalar values. I.e. this doesn't work:
import pandas as pd
import xarray as xr
pd.to_datetime(xr.DataArray(data=np.datetime64("2005-01-01")))
# TypeError: len() of unsized object
but this does:
import numpy as np
import xarray as xr
np.asarray(xr.DataArray(data=np.datetime64("2005-01-01")))
# array('2005-01-01T00:00:00.000000000', dtype='datetime64[ns]')
We actually came across this before in #464 (comment).
) | ||
for index, item in enumerate(value): | ||
try: | ||
assert " " not in str(item) |
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'm a little confused how the assert
statement works. Could you please explain a little bit?
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.
Sorry that this isn't very readable, it's following the EAFP style, usually used when we're handling only a few exceptional cases. The original reason that we can't pass in 'datetime'-like values to the 'region' argument is because there's a space " "
when converting a pandas.Timestamp
to a string (instead of a "T"
that would make it ISO compliant).
import pandas as pd
str(pd.Timestamp("2015-01-01T12:00:00.123456789"))
# '2015-01-01 12:00:00.123456789'
This also applies to xarray.DataArray
datetime values. So the assert
here checks for the space " "
, and if it finds it, it will try to convert the item into an ISO compliant datetime value (e.g. "2015-01-01T12:00:00.123456789"
).
kwargs[arg] = separators[fmt].join( | ||
"{}".format(item) for item in value | ||
) | ||
for index, item in enumerate(value): |
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 if I understand your codes correctly. It seems the codes check all arguments. For example, perspective=[30, 50] is converted to -p30/50, and the codes also check if 30
and 50
are datetime-like, right?
However, -p doesn't accept any datetime-like values. It looks a waste of time. Perhaps we should check whether arg="B"
(maybe arg="region")?
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 if I understand your codes correctly. It seems the codes check all arguments. For example, perspective=[30, 50] is converted to -p30/50, and the codes also check if
30
and50
are datetime-like, right?
Yes, we do check every item using assert " " not in str(item)
, but this is quite a fast string parsing check (see also the other comment on EAFP syntax). It is not explictily checking for datetime-like (which would be a slower check) using something like isinstance(item, (pd.Timestamp, datetime.datetime, etc))
.
However, -p doesn't accept any datetime-like values. It looks a waste of time. Perhaps we should check whether
arg="B"
(maybe arg="region")?
This would be an extra check on top of the (rather fast) assert " " not in str(item)
, and yes, we could add it I suppose. But really, there's usually not very many items to parse here, only 4 if using [xmin, xmax, ymin, ymax]
, maybe 6 if using [xmin, xmax, ymin, ymax, zmin, zmax]
.
Happy to consider an alternative way though if you have any ideas, I definitely think there's a better way to do this. The unit tests are all there, so feel free to refactor/change the code as long as it passes the tests!
"{}".format(item) for item in value | ||
) | ||
for index, item in enumerate(value): | ||
try: |
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.
because there's a space " " when converting a pandas.Timestamp to a string
Please add it near line 360 to make the assert
code easier to understand.
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.
The PR looks good to me.
Description of proposed changes
Allow users to pass in datetime-like inputs to the 'region' argument in modules like
plot
,basemap
, and so on. Helpful for people who usually just want to use something likeregion = [data.min(), data.max(), ...]
.Includes support for:
What is not supported:
1/1/2018
,Jul 5, 2019
Fixes #561
Reminders
make format
andmake check
to make sure the code follows the style guide.doc/api/index.rst
.