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: func -> built-in conversion for non-trig. methods #308

Conversation

sadielbartholomew
Copy link
Member

@sadielbartholomew sadielbartholomew commented Feb 2, 2022

Partially address #302, to tick all sub-issue boxes except the topmost* and in doing so close #303, since Data.log is now fully migrated (and marked as such) because the /= operator it uses with a non-e/10/2 based is applied in 'Dask space' rather than cf space.

Note

* the topmost item, 'the eight trigonometric and hyperbolic methods and their inverses which don't have restricted domains', will be tackled as a separate PR to make it easier for the reviewer to check that the correct eight methods have been converted and in a consistent way, with commenting to explain why the other four such methods have been left as func calls. And since the code changes required on all of the methods listed in #302 are largely the same, such that any review feedback can be done as one on them all, and it would be more time-consuming to make seven separate PRs for each, I thought it best to split into just two PRs to cover the Issue (second PR for the trig. methods to follow).

@sadielbartholomew sadielbartholomew added performance Relating to speed and memory performance dask Relating to the use of Dask labels Feb 2, 2022
@sadielbartholomew sadielbartholomew self-assigned this Feb 2, 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.

Nice job. A few minor suggestions, but all tests pass for me. After you've dealt with the comments (in whatever way you want), please go ahead and merge.

cf/data/data.py Outdated Show resolved Hide resolved
cf/data/data.py Show resolved Hide resolved
cf/test/test_Data.py Show resolved Hide resolved
@sadielbartholomew
Copy link
Member Author

Quickly re-running he CI jobs to double check they are still passing, triggering via open-close...

@sadielbartholomew
Copy link
Member Author

All checks passing, good to go...

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 performance Relating to speed and memory performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants