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.

Sign up for GitHub

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

Jump to bottom

Allow datetime inputs to region argument #562

Merged
merged 2 commits into from
Aug 16, 2020
Merged

Allow datetime inputs to region argument #562

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b74ff48
Support various datetime inputs to region argument
weiji14 Aug 14, 2020
71b4095
Note how datetime_to_string conversion relies on checking for a space
weiji14 Aug 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 28 additions & 3 deletions pygmt/helpers/decorators.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
import textwrap
import functools

import numpy as np

from .utils import is_nonstr_iter
from ..exceptions import GMTInvalidInput

Expand Down Expand Up @@ -302,6 +304,23 @@ def kwargs_to_strings(convert_bools=True, **conversions):
>>> module(123, bla=(1, 2, 3), foo=True, A=False, i=(5, 6))
{'bla': (1, 2, 3), 'foo': '', 'i': '5,6'}
args: 123
>>> import datetime
>>> module(
... R=[
... np.datetime64("2010-01-01T16:00:00"),
... datetime.datetime(2020, 1, 1, 12, 23, 45),
... ]
... )
{'R': '2010-01-01T16:00:00/2020-01-01T12:23:45.000000'}
>>> import pandas as pd
>>> import xarray as xr
>>> module(
... R=[
... xr.DataArray(data=np.datetime64("2005-01-01T08:00:00")),
... pd.Timestamp("2015-01-01T12:00:00.123456789"),
... ]
... )
{'R': '2005-01-01T08:00:00.000000000/2015-01-01T12:00:00.123456'}

"""
valid_conversions = [
Expand Down Expand Up @@ -338,9 +357,15 @@ def new_module(*args, **kwargs):
value = kwargs[arg]
issequence = fmt in separators
if issequence and is_nonstr_iter(value):
kwargs[arg] = separators[fmt].join(
"{}".format(item) for item in value
)
for index, item in enumerate(value):
Copy link
Member

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")?

Copy link
Member Author

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?

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!

try:
Copy link
Member

@seisman seisman Aug 16, 2020 •
edited
Loading

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.

assert " " not in str(item)
Copy link
Member

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?

Copy link
Member Author

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").

except AssertionError:
# convert datetime-like items to string format
value[index] = np.datetime_as_string(
np.asarray(item, dtype=np.datetime64)
Copy link
Member

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.

pygmt/pygmt/clib/session.py

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?

Copy link
Member Author

@weiji14 weiji14 Aug 15, 2020 •
edited
Loading

Choose a reason for hiding this comment