-
Notifications
You must be signed in to change notification settings - Fork 220
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
Support timedelta64 dtype as input #2884
Conversation
Map np.timedelta64 to GMT_LONG in clib/session.py's DTYPES dictionary. Added a unit test in test_clib_put_vectors.py to test passing a numpy array with timedelta64 dtypes of various time units (year to microsecond).
Make a 2D plot with Forecast Days (timedelta64) on the x-axis, and RMSE on the y-axis.
fig = Figure() | ||
fig.basemap( | ||
projection="X8c/5c", | ||
region=[0, 8, 0, 10], |
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.
Ideally, we could pass in np.timedelta64
into the region argument as mentioned at #2848 (comment).
region=[0, 8, 0, 10], | |
region=[np.timedelta64(0, "D"), np.timedelta64(8, "D"), 0, 10], |
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.
Ah ok, looks like I modified kwargs_to_strings
3 years ago at #562 to handle datetime
objects in region
, so could probably do something similar here for timedelta64
.
Summary of changed imagesThis is an auto-generated report of images that have changed on the DVC remote
Image diff(s)Report last updated at commit 4426d95 |
Was using 9.56.1 before.
Cast np.timedelta64 inputs to int, so that they can be understood by GMT.
pygmt/helpers/decorators.py
Outdated
if getattr( | ||
getattr(item, "dtype", ""), "name", "" | ||
).startswith("timedelta"): | ||
# A np.timedelta64 item is cast to integer | ||
value[index] = item.astype("int") |
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.
This double getattr to obtain item.dtype.name
isn't very nice, wondering if there's a good alternative.
pygmt/helpers/decorators.py
Outdated
... np.timedelta64(0, "h"), | ||
... np.timedelta64(24, "h"), | ||
... ] | ||
... ) | ||
{'R': '2010-01-01T16:00:00/2020-01-01T12:23:45.000000'} | ||
{'R': '2010-01-01T16:00:00/2020-01-01T12:23:45.000000/0/24'} |
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.
Apparently Python has a built-in datetime.timedelta
objects, but it's super hard to cast it to an integer type while preserving the orignal unit (e.g. day, hour, minute, etc). E.g.
import datetime
td = datetime.timedelta(hours=24)
print(np.timedelta64(td))
# numpy.timedelta64(86400000000,'us')
print(np.timedelta64(td).astype(int))
# 86400000000
Should we still support datetime.timedelta
? Or only np.timedelta64
?
pygmt/helpers/decorators.py
Outdated
if " " in str(item): | ||
# Check if there is a space " " when converting | ||
# a pandas.Timestamp/xr.DataArray to a string. | ||
# If so, use np.datetime_as_string instead. | ||
# Convert datetime-like item to ISO 8601 | ||
# string format like YYYY-MM-DDThh:mm:ss.ffffff. | ||
value[index] = np.datetime_as_string( | ||
np.asarray(item, dtype=np.datetime64) | ||
) | ||
# Check if there is a space " " in the item, which | ||
# is typically present in objects such as | ||
# np.timedelta64, pd.Timestamp, or xr.DataArray. | ||
# If so, convert the item to a numerical or string | ||
# type that is understood by GMT as follows: | ||
if getattr( | ||
getattr(item, "dtype", ""), "name", "" | ||
).startswith("timedelta"): | ||
# A np.timedelta64 item is cast to integer | ||
value[index] = item.astype("int") | ||
else: | ||
# A pandas.Timestamp/xr.DataArray containing | ||
# a datetime-like object is cast to ISO 8601 | ||
# string format like YYYY-MM-DDThh:mm:ss.ffffff | ||
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.
The if-then logic here might get a lot more complicated if we also decide to support passing PyArrow time/date types (xref #2800) into region
/-R
. At such, it might be good to isolate this logic into a separate function instead, so I'm thinking of maybe reverting commit d5bedc2 for now, but cherry-pick that change into a separate PR.
Specifically, we might want to create a dedicated _cast_datetime_to_str
function. There's actually something similar used in solar
(see 6406b43), so we could potentially re-use the function in many places.
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.
Sounds good.
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.
Ok, commit reverted in 849feda. Should be ready to review now.
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.
Looks good, but maybe wait for #2694?
Description of proposed changes
Map
np.timedelta64
toGMT_LONG
in clib/session.py's DTYPES dictionary. Added a unit test in test_clib_put_vectors.py to test passing a numpy array with timedelta64 dtypes of various time units (year to microsecond).TODO:
np.timedelta64
viaput_vector
np.timedelta64
min/max values in theregion
parameterExample usage:
produces
Addresses #2848, extends #464 and #562.
Reminders
make format
andmake check
to make sure the code follows the style guide.doc/api/index.rst
.Slash Commands
You can write slash commands (
/command
) in the first line of a comment to performspecific operations. Supported slash commands are:
/format
: automatically format and lint the code/test-gmt-dev
: run full tests on the latest GMT development version