Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dask: Data_asreftime; Data._asdatetime; Data.year & friends #322

Merged
merged 3 commits into from
Feb 28, 2022

Conversation

davidhassell
Copy link
Collaborator

No description provided.

Copy link
Member

@sadielbartholomew sadielbartholomew left a comment

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
Copy link
Member

@sadielbartholomew sadielbartholomew Feb 23, 2022

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...

Suggested change
>>> import numpy as np


**Examples**

>>> import numpy as np
Copy link
Member

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:

Suggested change
>>> import numpy as np


**Examples**

>>> import numpy as np
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
>>> import numpy as np

cf/data/data.py Outdated Show resolved Hide resolved
cf/test/test_Data.py Outdated Show resolved Hide resolved
davidhassell and others added 2 commits February 28, 2022 08:43
Co-authored-by: Sadie L. Bartholomew <sadie.bartholomew@ncas.ac.uk>
Co-authored-by: Sadie L. Bartholomew <sadie.bartholomew@ncas.ac.uk>
@davidhassell
Copy link
Collaborator Author

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!

@davidhassell
Copy link
Collaborator Author

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'

@davidhassell
Copy link
Collaborator Author

davidhassell commented Feb 28, 2022

... 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.

@davidhassell davidhassell merged commit 7b17e46 into NCAS-CMS:lama-to-dask Feb 28, 2022
@sadielbartholomew
Copy link
Member

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.

@davidhassell
Copy link
Collaborator Author

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

@davidhassell davidhassell deleted the dask-year branch November 15, 2022 09:16
@davidhassell davidhassell added this to the 3.14.0 milestone Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dask Relating to the use of Dask
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants