Decoding `cftime_variables` #122

jsignell · 2024-05-20T20:16:29Z

Closes Option to interpret variables using cftime #117
Tests added
Changes are documented in docs/releases.rst
New functions/methods are listed in api.rst

This is what I have started working on for the cftime_variables. I am going to be on vacation for a week so I thought I should show what I've gotten up to in case there is some urgency around this, but it is pretty WIP 🥲

dcherian · 2024-05-21T22:27:17Z

virtualizarr/kerchunk.py

+                calendar = var.attrs.get(
+                    "calendar", var.encoding.get("calendar", "standard")
                )
+                units = var.attrs.get("units", var.encoding["units"])
+
+                data = cftime.date2num(var, calendar=calendar, units=units).ravel()
+                var = var.copy(data=data)
+                np_arr = var.to_numpy()


Thinking out loud: Should this be a Zarr Codec instead?

naive question: do you mean a new CFTime codec or just a number codec rather than numpy?

Would that just be a refactoring in this PR? Like just separate out the logic here into a Codec class that could later be moved upstream?

@TomNicholas I've looked at a tiny portion of upstreaming CF logic to a codec for CF scale_factor and add_offset but this case has the benefit of a pre-existing numcodec FixedScaleAndOffset.

There is also a CFScaleOffsetCoder in xarray too...

jsignell · 2024-06-05T20:11:28Z

Ok! I think the issue was that I was encoding the var to a number but then I wasn't actually storing the encoding information in zarr 😬 . So now that that is resolved things seem to be working better but @jbusecke if you have a test you want to try go for it!

virtualizarr/xarray.py

jbusecke · 2024-06-11T22:39:01Z

If folks here have a minute, I just tested this PR over at jbusecke/esgf-virtual-zarr-data-access#10 with real world CMIP data. I think it generally works, but the attributes of the resulting time coordinate are different. Would love to get some input on that.

TomNicholas

I would love to get this merged!

This is an important feature, so deserves a note in the usage documentation too.

virtualizarr/kerchunk.py

virtualizarr/tests/test_integration.py

virtualizarr/xarray.py

jsignell · 2024-06-21T15:56:45Z

Thanks for the review! I think I have addressed all the comments.

jsignell · 2024-06-21T15:57:15Z

virtualizarr/tests/test_integration.py

-        roundtrip = xr.open_dataset(f"{tmpdir}/refs.{format}", engine="kerchunk")
+        roundtrip = xr.open_dataset(
+            f"{tmpdir}/refs.{format}", engine="kerchunk", decode_times=decode_times
+        )

        # assert equal to original dataset
        xrt.assert_equal(roundtrip, ds)


This test doesn't pass yet, but it's getting a lot closer! It's basically a floating point difference at this point. I tried switching to xrt.assert_allclose but I'm not sure it understands times because it still raises. If I do (roundtrip.time - ds.time).sum() I get zero so I do think the times are pretty darn close.

I tried using numpy directly to get to the bottom of things:

>>> from numpy.testing import assert_allclose >>> assert_allclose(roundtrip.time.values, ds.time.values) *** numpy.exceptions.DTypePromotionError: The DType <class 'numpy.dtypes.DateTime64DType'> could not be promoted by <class 'numpy.dtypes._PyFloatDType'>. This means that no common DType exists for the given inputs. For example they cannot be stored in a single array unless the dtype is `object`. The full list of DTypes is: (<class 'numpy.dtypes.DateTime64DType'>, <class 'numpy.dtypes._PyFloatDType'>)

See 0ffce4f for a rather silly solution

jsignell · 2024-06-21T18:21:48Z

virtualizarr/utils.py

@@ -113,7 +113,7 @@ def recode_cftime(var: xr.Variable) -> NDArray[cftime.datetime]:
    for c in var.values:
        value = cftime.num2date(
            cftime.date2num(
-                datetime.datetime.fromisoformat(str(c)),
+                datetime.datetime.fromisoformat(str(c.astype("M8[us]"))),


For Python 3.10 microsecond seems to be the smallest supported unit.

Changed in version 3.11: Previously, this method only supported formats that could be emitted by date.isoformat() or datetime.isoformat().

ref https://docs.python.org/3/library/datetime.html#datetime.datetime.fromisoformat

jsignell · 2024-06-21T19:03:46Z

Ok @TomNicholas I am pausing. Let me know how this is looking to you

TomNicholas · 2024-06-23T20:10:46Z

Thanks for going so deep into this @jsignell! To add more confusion - do you think what I'm seeing in #154 (comment) is related to your close-but-not-equal issue?

TomNicholas · 2024-06-24T21:27:29Z

@jsignell you might want to merge in #154 and #158.

Also sorry to push the finish line farther away, but what do you think about the idea in #117 (comment)?

jsignell · 2024-06-26T16:52:00Z

@jsignell you might want to merge in #154 and #158.

✅

Also sorry to push the finish line farther away, but what do you think about the idea in #117 (comment)?

No worries, better to do this the right way. I modeled the approach in this PR on kerchunk, but I will take a look at what is going on in xarray.

TomNicholas · 2024-06-26T17:04:13Z

No worries, better to do this the right way. I modeled the approach in this PR on kerchunk, but I will take a look at what is going on in xarray.

❤️

So I was looking at this in more detail last night, and I think we might just want to replace your encode_cftime and recode_cftime with importing xarray's CFDatetimeCoder (and possibly the CFTimeDeltaEncoder too). As far as I can tell (and please tell me if I'm wrong) those are basically more mature versions of the functions you've written here. Those classes are private but stable interfaces, and using them here could be an argument for factoring them out of xarray at some point (or exposing them more publicly).

Those encoders are called by xr.decode_cf here, but replacing everything with xr.decode_cf seems like a longer-term idea that is beyond the scope of this PR (but using xarray's encoders would build towards it).

jsignell · 2024-06-26T21:37:53Z

Ok I think I've finally got it working.

jsignell · 2024-06-26T21:39:27Z

virtualizarr/xarray.py

@@ -127,7 +148,9 @@ def open_virtual_dataset(
                filepath=filepath, reader_options=reader_options
            )

-            ds = xr.open_dataset(fpath, drop_variables=drop_variables)
+            ds = xr.open_dataset(
+                fpath, drop_variables=drop_variables, decode_times=False


This is a change that required changes to tests (see: cebb031) but I do think this is more correct. We don't want to be decoding things if it isn't explicitly requested right?

@TomNicholas really want to draw attention to this

I agree. We can also make this more auto-magic later if we want. I'm not sure any of the existing tests were covering this.

virtualizarr/kerchunk.py

jsignell · 2024-06-28T12:54:05Z

Let me know if you are looking for any more changes or feedback! Otherwise this is done from my perspective 😸

TomNicholas · 2024-06-28T17:50:11Z

Great contribution! Thanks so much @jsignell !

Try to get cftime_variables decoding

6cabcf2

dcherian reviewed May 21, 2024

View reviewed changes

TomNicholas added the encoding label May 21, 2024

sharkinsspatial mentioned this pull request May 23, 2024

How to handle encoding #68

Open

jbusecke mentioned this pull request Jun 3, 2024

Time encoding is lost jbusecke/esgf-virtual-zarr-data-access#8

Open

jsignell added 4 commits June 4, 2024 21:21

Merge branch 'main' into cftime

29be40a

Include encoding in zattrs

201c578

Add comment to test

96d393b

Merge branch 'main' into cftime

8642676

jsignell marked this pull request as ready for review June 5, 2024 20:09

jsignell changed the title ~~[WIP] Decoding cftime_variables~~ Decoding cftime_variables Jun 5, 2024

jsignell commented Jun 5, 2024

View reviewed changes

virtualizarr/xarray.py Outdated Show resolved Hide resolved

TomNicholas mentioned this pull request Jun 11, 2024

Aspirational use case: [C]Worthy mCDR OAE Atlas dataset #132

Open

21 tasks

jbusecke mentioned this pull request Jun 11, 2024

Testing time decoding jbusecke/esgf-virtual-zarr-data-access#10

Open

TomNicholas reviewed Jun 18, 2024

View reviewed changes

Merge branch 'main' into cftime

f200ef0

jsignell temporarily deployed to test-release June 21, 2024 13:17 — with GitHub Actions Inactive

jsignell commented Jun 21, 2024

View reviewed changes

virtualizarr/xarray.py Outdated Show resolved Hide resolved

Move functions into utils, parametrize test

88ce470

jsignell temporarily deployed to test-release June 21, 2024 14:50 — with GitHub Actions Inactive

Improve docstrings/errors

2f34ec9

jsignell temporarily deployed to test-release June 21, 2024 15:53 — with GitHub Actions Inactive

jsignell commented Jun 21, 2024

View reviewed changes

Make a hacky assert_allclose

0ffce4f

jsignell temporarily deployed to test-release June 21, 2024 17:49 — with GitHub Actions Inactive

Use us precision so that Python 3.10 works

66c359e

jsignell temporarily deployed to test-release June 21, 2024 18:20 — with GitHub Actions Inactive

jsignell commented Jun 21, 2024

View reviewed changes

Merge branch 'main' into cftime

d5f933e

jsignell temporarily deployed to test-release June 21, 2024 18:22 — with GitHub Actions Inactive

Add note to release doc

1dcce8f

jsignell temporarily deployed to test-release June 21, 2024 18:26 — with GitHub Actions Inactive

jsignell requested a review from TomNicholas June 21, 2024 19:03

TomNicholas mentioned this pull request Jun 23, 2024

Ensure attributes on coordinate variables are preserved during round-tripping #154

Merged

4 tasks

Merge branch 'main' into cftime

28dec88

jsignell temporarily deployed to test-release June 26, 2024 16:51 — with GitHub Actions Inactive

jsignell added 3 commits June 26, 2024 17:32

Use xarray Encoder and don't allow implicit decode_times

0467255

Merge branch 'main' into cftime

07d9dca

Fix up existing tests

cebb031

jsignell temporarily deployed to test-release June 26, 2024 21:39 — with GitHub Actions Inactive

jsignell commented Jun 26, 2024

View reviewed changes

Removing print statments

21c9ba1

TomNicholas approved these changes Jun 26, 2024

View reviewed changes

virtualizarr/kerchunk.py Outdated Show resolved Hide resolved

jsignell temporarily deployed to test-release June 26, 2024 21:42 — with GitHub Actions Inactive

TomNicholas merged commit 10e7863 into zarr-developers:main Jun 28, 2024
7 checks passed

jsignell deleted the cftime branch June 28, 2024 18:55

This was referenced Jul 1, 2024

Usage docs example of using cftime_variables #169

Closed

Despite more times available, dataset read from MultiZarrToZarr returns specific number time steps fsspec/kerchunk#343

Closed

TomNicholas added a commit that referenced this pull request Jul 1, 2024

un-xfail test that should have been un-xfailed in #122

08ca0cd

Decoding cftime_variables #122

Decoding cftime_variables #122

Uh oh!

Conversation

jsignell commented May 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsignell commented Jun 5, 2024

Uh oh!

Uh oh!

jbusecke commented Jun 11, 2024

Uh oh!

TomNicholas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jsignell commented Jun 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsignell commented Jun 21, 2024

Uh oh!

TomNicholas commented Jun 23, 2024

Uh oh!

TomNicholas commented Jun 24, 2024

Uh oh!

jsignell commented Jun 26, 2024

Uh oh!

TomNicholas commented Jun 26, 2024

Uh oh!

jsignell commented Jun 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jsignell commented Jun 28, 2024

Uh oh!

Uh oh!

TomNicholas commented Jun 28, 2024

Uh oh!

Uh oh!

Decoding `cftime_variables` #122

Decoding `cftime_variables` #122

jsignell commented May 20, 2024 •

edited

Loading