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

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

Conversation

sadielbartholomew
Copy link
Member

@sadielbartholomew sadielbartholomew commented Jun 15, 2022

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 sadielbartholomew added the dask Relating to the use of Dask label Jun 15, 2022
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?

cf/data/data.py Outdated Show resolved Hide resolved
cf/data/data.py Outdated Show resolved Hide resolved
cf/data/data.py Outdated Show resolved Hide resolved
cf/data/data.py Outdated Show resolved Hide resolved
cf/data/data.py Outdated Show resolved Hide resolved
cf/data/data.py Outdated Show resolved Hide resolved
cf/data/data.py Outdated Show resolved Hide resolved
cf/data/data.py Outdated Show resolved Hide resolved
cf/data/data.py Outdated Show resolved Hide resolved
cf/data/data.py Outdated Show resolved Hide resolved
@sadielbartholomew sadielbartholomew marked this pull request as ready for review June 17, 2022 17:32
@sadielbartholomew
Copy link
Member Author

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

(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

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