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.masked_invalid, Data.mask_invalid #390

Merged
merged 7 commits into from
May 4, 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.

All good bar one typo, though I was wondering a few things relating to this:

  • is editing the mixin.properties{data, databounds} within scope for these Dask migrations? If so, it looks like there are some example snippets to update mask_ to masked_ (and also some in data.data itself) so it would be good to blitz those:

    $ pwd
    /home/sadie/cf-python/cf
    $ git grep "mask_invalid"
    data/data.py:        >>> d.mask_invalid(inplace=True)
    data/data.py:        >>> d.mask_invalid(inplace=True)
    data/data.py:        >>> d.mask_invalid(inplace=True)
    data/data.py:        >>> d.mask_invalid(inplace=True)
    data/mixin/deprecations.py:        `mask_invalid` method.
    data/mixin/deprecations.py:        .. seealso:: `cf.Data.seterr`, `mask_invalid`
    data/mixin/deprecations.py:    def mask_invalid(self, *args, **kwargs):
    data/mixin/deprecations.py:        >>> f.mask_invalid()
    data/mixin/deprecations.py:            "mask_invalid",
    data/mixin/deprecations.py:        .. seealso:: `cf.Data.mask_fpe`, `mask_invalid`
    mixin/propertiesdata.py:    def mask_invalid(self, inplace=False, i=False):
    mixin/propertiesdata.py:          might be faster than calling `mask_invalid`.
    mixin/propertiesdata.py:        >>> h.mask_invalid(inplace=True)
    mixin/propertiesdata.py:        >>> h.mask_invalid(inplace=True)
    mixin/propertiesdata.py:            data.mask_invalid(inplace=True)
    mixin/propertiesdata.py:        >>> f.mask_invalid(inplace=True)
    mixin/propertiesdata.py:        >>> f.mask_invalid(inplace=True)
    mixin/propertiesdata.py:        >>> f.mask_invalid(inplace=True)
    mixin/propertiesdata.py:        >>> f.mask_invalid(inplace=True)
    mixin/propertiesdatabounds.py:    def mask_invalid(self, inplace=False, i=False):
    mixin/propertiesdatabounds.py:          might be faster than calling `mask_invalid`.
    mixin/propertiesdatabounds.py:        >>> h.mask_invalid(inplace=True)
    mixin/propertiesdatabounds.py:        >>> h.mask_invalid(inplace=True)
    mixin/propertiesdatabounds.py:            "mask_invalid",
    mixin/propertiesdatabounds.py:        >>> f.mask_invalid(inplace=True)
    mixin/propertiesdatabounds.py:        >>> f.mask_invalid(inplace=True)
    mixin/propertiesdatabounds.py:        >>> f.mask_invalid(inplace=True)
    mixin/propertiesdatabounds.py:        >>> f.mask_invalid(inplace=True)
    test/test_AuxiliaryCoordinate.py:    def test_AuxiliaryCoordinate_mask_invalid(self):
    test/test_AuxiliaryCoordinate.py:        a.mask_invalid()
    test/test_AuxiliaryCoordinate.py:        self.assertIsNone(a.mask_invalid(inplace=True))
    test/test_AuxiliaryCoordinate.py:        a.mask_invalid()
    test/test_AuxiliaryCoordinate.py:        self.assertIsNone(a.mask_invalid(inplace=True))
    test/test_Field.py:    def test_Field_mask_invalid(self):
    test/test_Field.py:        self.assertIsNone(f.mask_invalid(inplace=True))
  • are we keeping tabs on the API changes we are making during this dask migration stage (not including the proposed ones after that ready for v4.0.0)? Assuming we aren't yet, it would be good to start noting these down ready for the resultant change log entry. I guess we can use our big table Replace LAMA with Dask to parallelise cf.Data #182 and the additions to the deprecations module from the overall lama-to-dask -> master eventual merge to work out most of the ones made, which shouldn't be many?

cf/data/mixin/deprecations.py Outdated Show resolved Hide resolved
Co-authored-by: Sadie L. Bartholomew <[email protected]>
@davidhassell
Copy link
Collaborator Author

Hi Sadie - I'm inclined to leave down stream changes (such as to mixin/propertiesdata.py) to seperate PRs, so as to keep the changes to Data self contained.

Keeping tabs on the API changes is definitely a good idea. Perhaps I'll make a start my adding some to the table in #295, from whence they could end up in other issues (like #182).

@sadielbartholomew
Copy link
Member

Thanks for the clarifications David.

I'm inclined to leave down stream changes (such as to mixin/propertiesdata.py) to seperate PRs, so as to keep the changes to Data self contained.

Very sensible. In which case, we can sort that post-merge to master, though maybe you can update the four cases of mask_invalid still contained in a docstring example under data.data itself, as shown in my code snippet above, so as not to advertise the deprecated method?

Keeping tabs on the API changes is definitely a good idea. Perhaps I'll make a start my adding some to the table in #295, from whence they could end up in other issues (like #182).

Great. I see you've updated the table to mark such changes in the third column (looks like I somehow guessed the right number of 'notes' columns we'd eventually have use for!), nice idea.

So, apart from perhaps addressing the minor issue of those four cases of mask_invalid in the data examples, which is optional, this is good to merge. Thanks :)

@davidhassell davidhassell merged commit 3050d1e into NCAS-CMS:lama-to-dask May 4, 2022
@davidhassell davidhassell deleted the dask-mask-invalid branch November 15, 2022 09:27
@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