issue_comments

Thanks @pierreloicq!

Sounds good. I think "nan" is more explicit than "".

I agree. For now, it would be good to use "nan" (or ""?).

The only way to really fix this would be to introduce missing values for string dtypes, but I guess that will still take a while (unless we get pandas extension arrays to behave like duck arrays and to support n-dimensional arrays, see #5287).

@dcherian @mathause @andersy005 given a DataArray of times with missing values, do you have any thoughts on what the preferred result of da.dt.season would be?

One option would be to return np.nan in place of the missing time values:

``` In [3]: times = [np.datetime64("NaT"), np.datetime64("2000-01-01")]

In [4]: da = xr.DataArray(times, dims=["x"])

In [5]: da.dt.season Out[5]: <xarray.DataArray 'season' (x: 2)> array([nan, 'DJF'], dtype=object) Dimensions without coordinates: x ```

This would be somewhat in line with how pandas handles this in other contexts (e.g. https://github.com/pydata/xarray/pull/5876#discussion_r734447120). But this sort of awkwardly returns a DataArray of mixed types. Another option, and this is how @pierreloicq implemented things originally, would be to simply return a string label for missing values, e.g.

In [5]: da.dt.season Out[5]: <xarray.DataArray 'season' (x: 2)> array(['nan', 'DJF'], dtype='<U32') Dimensions without coordinates: x

As I've thought about this more, this feels nicer, because the types are consistent and "season" is just a category label, and "nan" categorizes these values just as well as np.nan. Do you agree?

As I've thought about this more, this feels nicer, because the types are consistent and "season" is a just a category label, and "nan" categorizes these values just as well as np.nan.

I prefer this from a user perspective. It's nice not to have to deal with mixed types and object arrays.

No worries about the cftime tests by the way -- how to best handle missing values in that context is still somewhat of an open question (https://github.com/Unidata/cftime/issues/145).

I did not manage to add the test on the @requires_cftime def test_seasons: cftime_date_type(np.datetime64("NaT")) or cftime_date_type(np.nan) are not allowed

Feel free to add it if you understand that more than me

Unit Test Results

6 files           6 suites   53m 13s :stopwatch: 16 209 tests 14 473 :heavy_check_mark: 1 736 :zzz: 0 :x: 90 450 runs  82 270 :heavy_check_mark: 8 180 :zzz: 0 :x:

Results for commit 817b42f3.

:recycle: This comment has been updated with latest results.

that's a random error that sometimes happens with mamba in CI. I've restarted the CI, hopefully it will pass now.

I don't understand this test fail. My modifications pass the tests on my computer.

Hello @pierreloicq! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2021-10-26 07:47:43 UTC

The code linting errors are still there and needs to be fixed, you can copy those changes to get that check passing. We use black for the code formatting, so if you run it there should be no code formatting differences, or run the pre-commit which also runs black. See http://xarray.pydata.org/en/stable/contributing.html

Indeed, but it looks like they were corrected automatically, right ? If so, I need to wait 5875 to pass ?

Le jeu. 21 oct. 2021 à 20:29, Illviljan @.***> a écrit :

=========================== short test summary info ============================ FAILED xarray/tests/test_backends.py::TestPseudoNetCDFFormat::test_ict_format FAILED xarray/tests/test_backends.py::TestPseudoNetCDFFormat::test_ict_format_write = 2 failed, 13692 passed, 1110 skipped, 226 xfailed, 66 xpassed, 128 warnings in 549.50s (0:09:09) =

Will be fixed in #5875 https://github.com/pydata/xarray/pull/5875.

But there's a few code linting errors which pep8speaks is also complaining about.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pydata/xarray/pull/5876#issuecomment-948893303, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACDGY2VM4DWDE6ZXM2K63HDUIBL2FANCNFSM5GJQ3F2Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

=========================== short test summary info ============================ FAILED xarray/tests/test_backends.py::TestPseudoNetCDFFormat::test_ict_format FAILED xarray/tests/test_backends.py::TestPseudoNetCDFFormat::test_ict_format_write = 2 failed, 13692 passed, 1110 skipped, 226 xfailed, 66 xpassed, 128 warnings in 549.50s (0:09:09) = Will be fixed in #5875.

But there's a few code linting errors which pep8speaks is also complaining about.

Hello, I tried some rapid change without even cloning the code. Can someone look at the failing tests ? some don't seem related to my modification.