dask: `Data_asreftime`; `Data._asdatetime`; `Data.year` & friends by davidhassell · Pull Request #322

dask: `Data_asreftime`; `Data._asdatetime`; `Data.year` & friends by davidhassell · Pull Request #322 · NCAS-CMS/cf-python

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic. All good, save some typos and possibly one minor aspect as raised in-line. Thanks again David, please merge when ready (and feel free to mark the properties as @daskified as well if you would like to).


		Examples

		>>> import numpy as np

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably unnecessary and somewhat distracting - the example snippet is not doctest-able anyway as-is unless you instead use cf.data.dask_utils.cf_YMDmhs...


		Examples

		>>> import numpy as np

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the equivalent reason to the above:


		Examples

		>>> import numpy as np

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Co-authored-by: Sadie L. Bartholomew <sadie.bartholomew@ncas.ac.uk>

Co-authored-by: Sadie L. Bartholomew <sadie.bartholomew@ncas.ac.uk>

Hi Sadie, thanks for the review. I found that the @daskified decorator didn't work with methods decorated with @property:

# Not @daskified
>>> cf.Data([1, 2], 'days since 2000-01-01').second
<CF Data(2): [0, 0]>

# @daskified
>>> cf.Data([1, 2], 'days since 2000-01-01').second
<bound method daskified.<locals>.decorator.<locals>.wrapper of <CF Data(2): [2000-01-02 00:00:00, 2000-01-03 00:00:00]>>

so I'll leave that for now.

I'm inclined to leave the numpy imports :) I find it useful to for everything to be defined for the reader. Happy to be persuaded otherwise, though!

Whilst testing the decorator, I found that initialising a Data object with a size 1 datetime wasn't working. I'll put in a fix as another commit on this PR:

>>> cf.Data([1, 2], 'days since 2000-01-01')
<CF Data(2): [2000-01-02 00:00:00, 2000-01-03 00:00:00]>
>>> cf.Data([1], 'days since 2000-01-01')
traceback
    ...
AttributeError: 'cftime._cftime.DatetimeGregorian' object has no attribute 'dtype'

... OK, it's not in the __repr__ that the problem lies, and that has (probably) been fixed in another PR that I've got in the wings. So I'll merge this now, afterall.

Hi David, that's all fine to me!

Regarding:

I found that the @daskified decorator didn't work with methods decorated with @Property

I guess you tried to add it above the property decorators? I realised when creating @daskified that this order wouldn't work, so added a note to the docstring, which covers how it needs to be done in these cases:

Note: for properties the decorator must be placed underneath the property decorator so it is called before and not after it.

So hopefully just popping it below will work. To help us keep track I'll try adding those in now as a follow-up commit.

Ah ha! Thanks for showing me how to decorate the properties.