ENH: support for reading and writing datetimes with timezones #253

m-richards · 2023-06-03T07:14:50Z

This is an initial proof of concept into reading and writing datetimes with timezones by reading and writing strings from gdal, and parsing the strings on the pandas side, very directly following discussion and suggestions in #123 123.

I'm trying not to affect the numpy read api, as a this functionality doesn't make sense from the numpy layer, as it doesn't support timezones.

There is something missing from this, the numpy api should still be able to read datetimes with timezones and convert them correctly to UTC (currently timezones are ignored complete), but I think that can be a separate PR (perhaps it should even come before this).

There's also probably a fair bit of work to handle driver support for this, I noticed issues locally trying to write geopackages (using WSL with an old version of gdal). I had a quick look at the tests in fiona, I was hoping for a simple list of supported formats, but it's quite complex there). I figure though that's a secondary issue after figuring out the implementation.

jorisvandenbossche

Thanks a lot for looking into this! Added some comments / suggestions

pyogrio/_io.pyx

jorisvandenbossche · 2023-06-04T12:20:26Z

pyogrio/geopandas.py

+        res = pd.to_datetime(ser, format="ISO8601")
+        if res.dtype != "object":
+            res = res.dt.as_unit("ms")


We might need to try to detect if there are timezones or not (eg check the first (few) strings to see if they contain a "+" or "-"), because if so we have to pass utc=True to not get object dtype:

In [20]: pd.to_datetime(["2021/01/02 01:02:03+01:00", "2021/01/02 01:02:03+01:00"], format="ISO8601") Out[20]: DatetimeIndex(['2021-01-02 01:02:03+01:00', '2021-01-02 01:02:03+01:00'], dtype='datetime64[ns, UTC+01:00]', freq=None) In [21]: pd.to_datetime(["2021/01/02 01:02:03+01:00", "2021/01/02 01:02:03+02:00"], format="ISO8601") Out[21]: Index([2021-01-02 01:02:03+01:00, 2021-01-02 01:02:03+02:00], dtype='object')

(with longer timeseries, it is very normal to have mixed offsets for the same timezone, i.e. values with and without DST)

that's a good point, I checked with what we did in geopandas for the fiona IO and did the same thing as there to be consistent -> read without utc=True, if still object dtype then read with utc=True and see if that makes a difference.

I could also look at check for "+" and "-" as you suggest, I'd imagine that's faster, but maybe there are some edge cases?

jorisvandenbossche · 2023-06-04T12:31:52Z

pyogrio/tests/test_geopandas_io.py

+    )  # TODO this shouldn't be called datetime as string in the pandas layer,
+    #     should it even be accessible?


Unless we have some users asking for it, I wouldn't expose it in read_dataframe (so only in raw.read), just to keep it simpler in read_dataframe

pyogrio/geopandas.py

jorisvandenbossche · 2023-09-30T08:36:26Z

pyogrio/geopandas.py

+            ".*parsing datetimes with mixed time zones will raise.*",
+            FutureWarning,
+        )
+        res = pd.to_datetime(ser, **datetime_kwargs)


I think we still might want to add a try/except around this, for when this would start raising an error in the future (also in that case we should fall back to the utc=True path below

pyogrio/tests/test_geopandas_io.py

Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>

brendan-ward · 2023-10-04T15:25:38Z

Apologies for the very slow review here; I am slowly trying to get up to speed and looking for other alternatives to adding dtype info to the meta.

brendan-ward

Thanks @m-richards for all your work on this!

I'll readily admit I don't work with datetimes very much, so the details around that are a bit beyond my experience. However, your approach here seems like a reasonable way of getting around the limitations of datetime handling in numpy vs pandas. Adding dtypes to meta seems reasonable, as does ignoring it on in raw.write().

It might be a good idea to add some notes about this to introduction.md to guide users about when to use datetime_as_string and otherwise how the returned values will be dependent on the version of Pandas used (if I follow correctly).

pyogrio/geopandas.py

brendan-ward · 2023-10-05T00:44:35Z

pyogrio/geopandas.py

@@ -355,8 +401,24 @@ def write_dataframe(
    # TODO: may need to fill in pd.NA, etc
    field_data = []
    field_mask = []
+    # dict[str, np.array(datetime.datetime)] special case for dt-tz fields
+    timezone_cols_metadata = {}


Suggested change

timezone_cols_metadata = {}

gdal_tz_offsets = {}

Slight preference to prefix params specifically intended to pass things down to GDAL as gdal_, and since the only thing being stored here is the GDAL offset rather than other metadata, this rename seems a bit more clear (provided I follow correctly).

Yes you do follow correctly, the new name makes more sense. I think the name I had predates me passing down the offset only.

pyogrio/raw.py

brendan-ward · 2023-10-05T01:08:03Z

pyogrio/tests/test_geopandas_io.py

+        expected = pd.to_datetime(raw_expected, format="ISO8601").as_unit("ms")
+    else:
+        expected = pd.to_datetime(raw_expected)
+    expected = pd.Series(expected, name="col")


Suggested change

expected = pd.Series(expected, name="col")

expected = pd.Series(expected, name="datetime_col")

pyogrio/tests/test_geopandas_io.py

pyogrio/tests/test_raw_io.py

m-richards · 2023-10-07T05:31:16Z

Thanks @m-richards for all your work on this!

Thanks for the detailed review, I hope I have addressed most of the suggestions now.

It might be a good idea to add some notes about this to introduction.md to guide users about when to use datetime_as_string and otherwise how the returned values will be dependent on the version of Pandas used (if I follow correctly).

Happy to do this, but just want to double check the structuring. I am not super familiar with the pyogrio documentation, but introduction.md seems to all describe using read_dataframe. At present this pull request doesn't expose datetime_as_string to read_dataframe, only in raw.py::read so I don't know if it makes sense to document / whether it should be documented elsewhere?

On return types, happy to add this, but I saw there is already somewhat of a discussion of this at known_issues.md - which I now realise I need to update as part of this pull request too. Happy to have an example in the introduction as well though if you think that makes sense.

jorisvandenbossche

Looking good!

pyogrio/geopandas.py

pyogrio/tests/test_geopandas_io.py

jorisvandenbossche · 2023-10-07T09:44:07Z

At present this pull request doesn't expose datetime_as_string to read_dataframe, only in raw.py::read so I don't know if it makes sense to document / whether it should be documented elsewhere?

I would personally say it's good to keep this raw only, and then having the keyword explained in the docstrings seems good enough to me.

And the section in the "known_issues" about the datetime gotchas is already a nice and quite complete section I think. Maybe we could just move it "introduction", as now it's actually no longer a known "issue" with pyogrio, but general limitations of reading/writing datetimes because of GDAL's data model.

brendan-ward · 2023-10-10T16:34:32Z

Apologies for the poor direction. Leaving datetime_as_string at raw level only is fine. I'd suggest moving the datimetime section from known issues to the introduction.

pyogrio/_io.pyx

brendan-ward

Thanks for the updates @m-richards ! Getting really close. Looks like there are a few uncommitted suggestions, but after those are in this looks good to me.

Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>

m-richards · 2023-10-17T22:01:06Z

Thanks for the updates @m-richards ! Getting really close. Looks like there are a few uncommitted suggestions, but after those are in this looks good to me.

I think they're all done now, don't know how I missed them before.

brendan-ward · 2023-10-20T02:59:15Z

Thanks @m-richards ! 🚀

m-richards added 24 commits May 7, 2023 22:07

minimal working pandas layer without timezones

e075091

implement datetime_as_string toggle to get numpy layer working

3df7936

make tests pass

d68b473

add tests showing existing behaviour no tz

9aa5a8c

working read

1a2af4d

commit my test file

fbd2898

actually fix tests with read working

127d0a7

good enough wip progress for now

016778a

make these failures easier to read

faa0631

fix for non tz

a8c200e

fix some tests

6047375

run pre commit

6061563

maybe old pandas, can't reproduce locally

3ba42cf

try and find something pandas 1.5 also happy with

d983140

lint

e9993bd

simple answer

b6ca5cf

cleanup

05cc1cf

wip, use strings to make multi timezones round trip

a78a76c

use tmp path fixture

b681656

cleanups

3426fdc

try cleanup datetime parsing

bb6fd4e

more cleanup, realise we can get dt resolution

87419ac

more careful pandas 1.5 compat

fc78bd9

delete line

5fab348

m-richards mentioned this pull request Jun 3, 2023

Support timezones when reading and writing datetime columns #123

Open

jorisvandenbossche reviewed Jun 4, 2023

View reviewed changes

pyogrio/geopandas.py Outdated Show resolved Hide resolved

m-richards added 3 commits August 8, 2023 20:20

replace write support with working datetime object solution

26c403a

fixes

ebdb71b

rewrite datetime reading to handle mixed offset to utc

f46e716

fix fix

fccc8fb

jorisvandenbossche reviewed Sep 30, 2023

View reviewed changes

pyogrio/tests/test_geopandas_io.py Outdated Show resolved Hide resolved

jorisvandenbossche reviewed Sep 30, 2023

View reviewed changes

pyogrio/tests/test_geopandas_io.py Outdated Show resolved Hide resolved

m-richards and others added 2 commits September 30, 2023 19:48

Apply suggestions from code review

200cc1d

Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>

add suggested exception handling

ebfc01c

brendan-ward reviewed Oct 5, 2023

View reviewed changes

m-richards added 2 commits October 7, 2023 15:38

move pandas compat to _compat

c8c186a

address review comments

95030c0

m-richards added 3 commits October 7, 2023 16:32

Merge remote-tracking branch 'upstream/main' into matt/timezones_redo

c5c272b

update known issues

086e52e

reword

2b2dd5f

jorisvandenbossche approved these changes Oct 7, 2023

View reviewed changes

pyogrio/geopandas.py Outdated Show resolved Hide resolved

pyogrio/geopandas.py Outdated Show resolved Hide resolved

pyogrio/tests/test_geopandas_io.py Outdated Show resolved Hide resolved

brendan-ward reviewed Oct 10, 2023

View reviewed changes

pyogrio/_io.pyx Outdated Show resolved Hide resolved

m-richards added 3 commits October 17, 2023 21:45

move documentation

2167d0f

rename field as suggested

ab0fbf6

Merge remote-tracking branch 'upstream/main' into matt/timezones_redo

e3f4d6a

brendan-ward reviewed Oct 17, 2023

View reviewed changes

m-richards and others added 3 commits October 18, 2023 08:54

final missing gdal tz offset change

0f02115

Update pyogrio/tests/test_geopandas_io.py

52a922d

Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>

Apply suggestions from code review

7c99e51

Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>

add changelog entry

a5f5f9d

brendan-ward merged commit 2f6a679 into geopandas:main Oct 20, 2023
14 of 15 checks passed

m-richards mentioned this pull request Mar 3, 2024

BUG: undefined variable in timezone logic #370

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: support for reading and writing datetimes with timezones #253

ENH: support for reading and writing datetimes with timezones #253

m-richards commented Jun 3, 2023 •

edited

Loading

jorisvandenbossche left a comment

jorisvandenbossche Jun 4, 2023

m-richards Aug 8, 2023

jorisvandenbossche Jun 4, 2023

jorisvandenbossche Sep 30, 2023

brendan-ward commented Oct 4, 2023

brendan-ward left a comment

brendan-ward Oct 5, 2023

m-richards Oct 7, 2023

brendan-ward Oct 5, 2023

m-richards commented Oct 7, 2023

jorisvandenbossche left a comment

jorisvandenbossche commented Oct 7, 2023

brendan-ward commented Oct 10, 2023

brendan-ward left a comment

m-richards commented Oct 17, 2023

brendan-ward commented Oct 20, 2023

		) # TODO this shouldn't be called datetime as string in the pandas layer,
		# should it even be accessible?

	expected = pd.Series(expected, name="col")
	expected = pd.Series(expected, name="datetime_col")

ENH: support for reading and writing datetimes with timezones #253

ENH: support for reading and writing datetimes with timezones #253

Conversation

m-richards commented Jun 3, 2023 • edited Loading

jorisvandenbossche left a comment

Choose a reason for hiding this comment

jorisvandenbossche Jun 4, 2023

Choose a reason for hiding this comment

m-richards Aug 8, 2023

Choose a reason for hiding this comment

jorisvandenbossche Jun 4, 2023

Choose a reason for hiding this comment

jorisvandenbossche Sep 30, 2023

Choose a reason for hiding this comment

brendan-ward commented Oct 4, 2023

brendan-ward left a comment

Choose a reason for hiding this comment

brendan-ward Oct 5, 2023

Choose a reason for hiding this comment

m-richards Oct 7, 2023

Choose a reason for hiding this comment

brendan-ward Oct 5, 2023

Choose a reason for hiding this comment

m-richards commented Oct 7, 2023

jorisvandenbossche left a comment

Choose a reason for hiding this comment

jorisvandenbossche commented Oct 7, 2023

brendan-ward commented Oct 10, 2023

brendan-ward left a comment

Choose a reason for hiding this comment

m-richards commented Oct 17, 2023

brendan-ward commented Oct 20, 2023

m-richards commented Jun 3, 2023 •

edited

Loading