Skip to content

LAMA to Dask: arithmetic, logical & comparison operators #409

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.

Sign up for GitHub

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

Jump to bottom
Merged
sadielbartholomew merged 22 commits into from
Jun 20, 2022
Merged

LAMA to Dask: arithmetic, logical & comparison operators #409

sadielbartholomew merged 22 commits into from
Jun 20, 2022

Conversation

@sadielbartholomew
Copy link
Member

@sadielbartholomew sadielbartholomew commented Jun 15, 2022 •
edited
Loading

Convert all of the arithmetic, logical & comparison operators from LAMA to Dask. Please cross-reference with #295 for the precise listing of methods being migrated here, if necessary.

@sadielbartholomew
Comment to mark arithmetic & similar methods to be migrated to dask
885a6f7
@sadielbartholomew
Apply basic tidy to un-daskified '_binary_operation' method
516cd4d
@sadielbartholomew
Test-driven development: un-skip relevant test to monitor migration
84f0439
@sadielbartholomew
Apply converted units post-operation in _binary_arithmetic
2454108
@sadielbartholomew
Add outline of logic for _binary_arithmetic migrated to use Dask
8e74e0f
@sadielbartholomew
Add logic to perform operation not in-place in _binary_arithmetic
50e75b2
@sadielbartholomew
Skip test case failing on unmigrated method RE _binary_arithmetic
77911be
@sadielbartholomew
Add logic to perform operation in-place in _binary_arithmetic
735171d
@sadielbartholomew
Skip new test case failing on unmigrated method RE _binary_arithmetic
6c8cb19
@sadielbartholomew
Tidy updated _binary_arithmetic & mark as 'daskified'
9313d6a
@sadielbartholomew
Fix 2x undefined name errors caught in _binary_arithmetic
16649c7
@sadielbartholomew
Mark all arithmetic, logical & comparison operators as 'daskified'
ff847ef
@sadielbartholomew sadielbartholomew added the dask Relating to the use of Dask label Jun 15, 2022
davidhassell
davidhassell reviewed Jun 15, 2022
View reviewed changes
Copy link
Collaborator

@davidhassell davidhassell left a comment

Choose a reason for hiding this comment

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

Thanks, Sadie. A quick review, as I know that there is more work to done ...

notably since I am about to add a commit or two applying elemwise calls to the operations that give the results

I don't really understand what these elemwise calls could be for - am I missing something?

sadielbartholomew and others added 10 commits June 15, 2022 14:17
@sadielbartholomew @davidhassell
Update cf/data/data.py: _get_dask -> to_dask_array
5caa169 
Co-authored-by: David Hassell <davidhassell@users.noreply.github.com>
@sadielbartholomew @davidhassell
Update cf/data/data.py: _get_dask -> to_dask_array (2)
4775f13 
Co-authored-by: David Hassell <davidhassell@users.noreply.github.com>
@sadielbartholomew @davidhassell
Update cf/data/data.py: remove deprecated kwarg to _set_dask
7b1f543 
Co-authored-by: David Hassell <davidhassell@users.noreply.github.com>
@sadielbartholomew @davidhassell
Update cf/data/data.py: remove deprecated kwarg to _set_dask (2)
ad09bf7 
Co-authored-by: David Hassell <davidhassell@users.noreply.github.com>
@sadielbartholomew
Address feedback: improve commenting in _binary_arithmetic section
e832e85
@sadielbartholomew
Address feedback: remove redundant catch in _binary_arithmetic
7f05719
@sadielbartholomew
Address feedback: remove 2nd redundant catch in _binary_arithmetic
742e48c
@sadielbartholomew
Update calls from cf's custom _numpy_isclose to da.isclose
6f7d1d7
@sadielbartholomew
Deprecate (internal => remove) cf's now-redundant _numpy_isclose
2e71011
@sadielbartholomew
Merge branch 'lama-to-dask' into lama-to-dask-binary-unary-arithmetic
3930cb7
@sadielbartholomew sadielbartholomew marked this pull request as ready for review June 17, 2022 17:32
@sadielbartholomew
Copy link
Member Author

sadielbartholomew commented Jun 17, 2022

Thanks for all of your feedback (so far) @davidhassell, which I have now addressed fully as far as I can tell.

Unless you have further comments, this should be ready to merge now, though I would like to also first check that I was OK to 'deprecate' our custom function _numpy_isclose by just removing it, rather than removing the functional code inside it and adding a deprecation warning, due to it being an internal function?

@sadielbartholomew
Copy link
Member Author

sadielbartholomew commented Jun 17, 2022 •
edited
Loading

(CI jobs failing or cancelled due to trying to use the wrong cfdm version as a dependency, which I will fix in the workflow but outside of this PR; overall we don't have to worry about it here. Locally the relevant test, test_Data.py, passes for me.)

@davidhassell
Copy link
Collaborator

davidhassell commented Jun 20, 2022

Hi Sadie - all looks good to me, thanks. As _numpy_isclose is an underscore method, I'd be fine with just deleting it. I've confirmed that the units tests pass on my laptop. Please merge!

@sadielbartholomew sadielbartholomew merged commit b974a4b into NCAS-CMS:lama-to-dask Jun 20, 2022
@sadielbartholomew sadielbartholomew deleted the lama-to-dask-binary-unary-arithmetic branch June 20, 2022 11:05
@sadielbartholomew sadielbartholomew mentioned this pull request Jun 21, 2022
Closed
@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

Reviewers

@davidhassell davidhassell davidhassell left review comments

Assignees

No one assigned

Labels

dask Relating to the use of Dask

Projects

None yet

Milestone

3.14.0

Development

Successfully merging this pull request may close these issues.

2 participants

@sadielbartholomew @davidhassell