Allow __getitem__ indices that have a to_dask_array method
#678
Conversation
sadielbartholomew
left a comment
There was a problem hiding this comment.
Works to re-implement the feature at hand, but ideally we can add to this PR to:
- add a little testing of the indices on the Field side in
test_Field__getitem__as well; and - also (in testing) cover cases of objects with masking as indices since masked arrays can be pesky;
- add documentation to highlight the possibility of using cf objects as indices, e.g. adding to the 'Assignment by index' section of the tutorial.
I've raised one minor comment and also have one question:
- should cf objects with a
to_dask_arraymethod now be valid inputs to theindicesmethods ofFieldandDomain(etc.) because they don't seem to work with those at the moment (but maybe they shouldn't and I am missing something!).
Changelog.rst
Outdated
| (https://github.com/NCAS-CMS/cf-python/issues/661) | ||
| * New keyword parameter to `cf.aggregate`: ``cells`` | ||
| (https://github.com/NCAS-CMS/cf-python/issues/452) | ||
| * Allow `__getitem__` indices that have a `to_dask_array` method |
There was a problem hiding this comment.
This message makes sense to us for dev purposes but doesn't strike me as being very user friendly, as opposed to something like, as a snippet from the opening sentence you made in that Issue:
... allow cf.Data and other objects (such as cf.DimensionCoordinate) to be used as indices ...
so how about putting something more like that instead? (Obviously imagine we are in an alternative universe where users read the Changelog 😃)
There was a problem hiding this comment.
As a firm adherent to the many-universes hypothesis, I can rest happy knowing that the obfuscated developers version lives on somewhere :)
There was a problem hiding this comment.
I'm looking at the improving the tests and docs as you have suggested ...
|
Fixes #677
Plus a bit of linting on some previous, unconnected work.