-
Notifications
You must be signed in to change notification settings - Fork 229
sequence_join: Correctly join datetime-like values in pd.DataFrame/xr.DataArray objects #4097
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
Conversation
….DataArray objects
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.
Pull Request Overview
This PR fixes the handling of datetime-like objects in the sequence_join
function to ensure they are properly converted to ISO 8601 string format instead of containing spaces that break GMT API compatibility. The fix adapts existing code from the kwargs_to_strings
decorator to detect datetime objects with spaces in their string representation and use np.datetime_as_string
for proper formatting.
- Adds logic to detect datetime-like objects that contain spaces when converted to strings
- Uses
np.datetime_as_string
to convert them to proper ISO 8601 format - Updates documentation with examples showing the new datetime handling capabilities
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
pygmt/helpers/utils.py | Adds numpy import and datetime conversion logic to sequence_join function with updated docstring examples |
pygmt/alias.py | Updates docstring with datetime handling examples for the _to_string function |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
pygmt/helpers/utils.py
Outdated
# If so, use np.datetime_as_string to convert it to ISO 8601 string format | ||
# YYYY-MM-DDThh:mm:ss.ffffff. | ||
for idx, item in enumerate(value): | ||
if " " 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.
Using ' ' in str(item)
as a heuristic to detect datetime objects is fragile and could produce false positives for non-datetime objects that happen to contain spaces in their string representation. Consider using isinstance
checks or hasattr
to detect datetime-like objects more reliably.
Copilot uses AI. Check for mistakes.
pygmt/helpers/utils.py
Outdated
# 'str(v)' produces a string like '2024-01-01 00:00:00' for some datetime-like | ||
# objects (e.g., datetime.datetime, pandas.Timestamp), which contains a space. | ||
# If so, use np.datetime_as_string to convert it to ISO 8601 string format | ||
# YYYY-MM-DDThh:mm:ss.ffffff. | ||
_value = [ | ||
np.datetime_as_string(np.asarray(item, dtype=np.datetime64)) | ||
if " " in str(item) | ||
else item | ||
for item in value | ||
] | ||
return sep.join(str(v) for v in _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.
We have some similar code from #2885/#562 here:
pygmt/pygmt/helpers/decorators.py
Lines 722 to 732 in bb6e084
for index, item in enumerate(value): | |
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) | |
) | |
newvalue = separators[fmt].join(f"{item}" for item in value) |
should there be a common function maybe to avoid duplication?
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.
Yes, the new codes are modified from the one you posted. I prefer to keep the kwargs_to_strings
decorator unchanged, since we're working towards removing this decorator.
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.
Hmm ok. I'm just trying to think of how to remove the double str()
call somehow. Something like this, but not exactly perfect:
# 'str(v)' produces a string like '2024-01-01 00:00:00' for some datetime-like | |
# objects (e.g., datetime.datetime, pandas.Timestamp), which contains a space. | |
# If so, use np.datetime_as_string to convert it to ISO 8601 string format | |
# YYYY-MM-DDThh:mm:ss.ffffff. | |
_value = [ | |
np.datetime_as_string(np.asarray(item, dtype=np.datetime64)) | |
if " " in str(item) | |
else item | |
for item in value | |
] | |
return sep.join(str(v) for v in _value) | |
# 'str(v)' produces a string like '2024-01-01 00:00:00' for some datetime-like | |
# objects (e.g., datetime.datetime, pandas.Timestamp), which contains a space. | |
# If so, use np.datetime_as_string to convert it to ISO 8601 string format | |
# YYYY-MM-DDThh:mm:ss.ffffff. | |
_values = [ | |
np.datetime_as_string(np.asarray(item, dtype=np.datetime64)) | |
if " " in str(item) | |
else str(item) | |
for item in value | |
] | |
return sep.join(_values) |
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've changed it to:
_values = [
np.datetime_as_string(np.asarray(item, dtype="datetime64"))
if " " in (s := str(item))
else s
for item in value
]
return sep.join(_values) # type: ignore[arg-type]
In the
sequence_join
function, we usestr(v)
to convert any value into a string that can be passed to the GMT API. However,str(v)
can't handle some datetime-like objects. For example,datetime.datetime(2010, 2, 1)
is converted to2010-02-01 00:00:00
with a whitespace. Below is an example showing the issue:This PR fixes the issue by adapting the existing codes in the
kwargs_to_strings
decorator:pygmt/pygmt/helpers/decorators.py
Lines 737 to 746 in 63d4561
With the changes, the above code produces the following output:
The changes are necessary to support arguments like
in the new alias system.