Implement cftime vectorization as discussed in PR #8322

[antscloud]

As discussed in #8322, here is the test for implementing the vectorization

Only this test seems to fail in test_coding_times.py : https://github.com/pydata/xarray/blob/f895dc1a748b41d727c5e330e8d664a8b8780800/xarray/tests/test_coding_times.py#L1061-L1071

I don't really understand why though if you have an idea

[headtr1ck]

@antscloud are you willing to continue this? It would be a good addition.

[antscloud]

Hi, sorry i completely forgot about this issue, i've implemented the suggestions and all the tests appear to pass, I hope it works in the CI 🤞

[headtr1ck]

BENCHMARKS NOT SIGNIFICANTLY CHANGED.

I didn't check in detail if we have a test that might cover this already and if yes, what the changes are.

[spencerkclark]

I don't think we currently do, but I think this new benchmark should cover it: https://github.com/pydata/xarray/pull/9262.

[headtr1ck]

| Change | Before [91a52dc7] | After [3512bee4] | Ratio | Benchmark (Parameter) | |----------|----------------------|---------------------|---------|-----------------------------------------------------------| | - | 139±0.9ms | 42.3±0.7ms | 0.3 | coding.EncodeCFDatetime.time_encode_cf_datetime('noleap') |

3 times faster, not bad

[headtr1ck]

The mypy error is still there, not sure how to effectively fix it, maybe use kwargs instead or simply type ignore it.

Otherwise this looks good and should get an entry in whats-new!

[kmuehlbauer]

Thanks @antscloud! Still a valuable addition, which should be mentioned in whats-new.rst. @antscloud are you still around to finalize this? Otherwise I could take care of that.

[kmuehlbauer]

Great, either I messed up the merge or this fails now, because of other concurring changes in the past time. @spencerkclark Do you spot anything suspicious here?

[spencerkclark]

Thanks @kmuehlbauer for reviving this—I think the test failures are a manifestation of the cftime issue you found earlier (https://github.com/Unidata/cftime/issues/354). The difference here from what was implemented in #9618 is that datetime.datetime objects are no longer converted to cftime.DatetimeProlepticGregorian objects prior to being passed to cftime.date2num.

While the issue was fixed in https://github.com/Unidata/cftime/pull/355, a new version of cftime has not yet been released. We could lobby for that, or raise an error in these circumstances. Independent of the cftime issue, there may be a reasonable case for raising (#10352).

[kmuehlbauer]

@spencerkclark Looks like this works after merging #10352. :tada:

[spencerkclark]

@kmuehlbauer I'm guessing you approve now, but giving you the chance to take another look and remove your requested change (which I think is resolved).

[spencerkclark]

The benchmark workflow failure is unrelated, so I am going to go ahead and merge. Thanks again @antscloud!