Skip to content

dask: Data.percentile, Data.median#313

Merged
sadielbartholomew merged 17 commits intoNCAS-CMS:lama-to-daskfrom
davidhassell:dask-percentile
Feb 9, 2022
Merged

dask: Data.percentile, Data.median#313
sadielbartholomew merged 17 commits intoNCAS-CMS:lama-to-daskfrom
davidhassell:dask-percentile

Conversation

@davidhassell
Copy link
Copy Markdown
Collaborator

@davidhassell davidhassell commented Feb 6, 2022

No description provided.

@davidhassell davidhassell mentioned this pull request Feb 6, 2022
Closed
@davidhassell davidhassell added the dask Relating to the use of Dask label Feb 7, 2022
sadielbartholomew
sadielbartholomew reviewed Feb 7, 2022
View reviewed changes
Copy link
Copy Markdown
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.

This is looking good generally, but I have some questions and comments...

sadielbartholomew
sadielbartholomew approved these changes Feb 8, 2022
View reviewed changes
Copy link
Copy Markdown
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.

In re-review I noticed some typos in docstrings and comments that could ideally be fixed, as raised in-line, but overall this appears to be watertight, with adapted and improved testing, and all feedback raised previously has been addressed well. Nice! Feel free to merge.

@sadielbartholomew
Copy link
Copy Markdown
Member

sadielbartholomew commented Feb 8, 2022

Actually, just thinking we should the check the CI jobs pre-merge, triggering those via open-close...

@sadielbartholomew
Copy link
Copy Markdown
Member

sadielbartholomew commented Feb 8, 2022 •
edited
Loading

The 3.7 job has failed on environment setup because of the new numpy>=1.22 requirement, where at v1.22.0 numpy has dropped Python 3.7. 3.7 isn't EOL until mid-2023, but I don't mind effectively dropping it. What do you think, @davidhassell (I guess the alternative is not to restrict to numpy>=1.22 after all)?

Collecting cfunits>=3.3.4
  Downloading cfunits-3.3.4.tar.gz (41 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 41.5/41.5 KB 1.9 MB/s eta 0:00:00
  Installing build dependencies: started
  Installing build dependencies: finished with status 'done'
  Getting requirements to build wheel: started
  Getting requirements to build wheel: finished with status 'done'
  Preparing metadata (pyproject.toml): started
  Preparing metadata (pyproject.toml): finished with status 'done'
ERROR: Could not find a version that satisfies the requirement numpy>=1.22 (from cf-python) (from versions: 1.3.0, 1.4.1, 1.5.0, 1.5.1, 1.6.0, 1.6.1, 1.6.2, 1.7.0, 1.7.1, 1.7.2, 1.8.0, 1.8.1, 1.8.2, 1.9.0, 1.9.1, 1.9.2, 1.9.3, 1.10.0.post2, 1.10.1, 1.10.2, 1.10.4, 1.11.0, 1.11.1, 1.11.2, 1.11.3, 1.12.0, 1.12.1, 1.13.0rc1, 1.13.0rc2, 1.13.0, 1.13.1, 1.13.3, 1.14.0rc1, 1.14.0, 1.14.1, 1.14.2, 1.14.3, 1.14.4, 1.14.5, 1.14.6, 1.15.0rc1, 1.15.0rc2, 1.15.0, 1.15.1, 1.15.2, 1.15.3, 1.15.4, 1.16.0rc1, 1.16.0rc2, 1.16.0, 1.16.1, 1.16.2, 1.16.3, 1.16.4, 1.16.5, 1.16.6, 1.17.0rc1, 1.17.0rc2, 1.17.0, 1.17.1, 1.17.2, 1.17.3, 1.17.4, 1.17.5, 1.18.0rc1, 1.18.0, 1.18.1, 1.18.2, 1.18.3, 1.18.4, 1.18.5, 1.19.0rc1, 1.19.0rc2, 1.19.0, 1.19.1, 1.19.2, 1.19.3, 1.19.4, 1.19.5, 1.20.0rc1, 1.20.0rc2, 1.20.0, 1.20.1, 1.20.2, 1.20.3, 1.21.0rc1, 1.21.0rc2, 1.21.0, 1.21.1, 1.21.2, 1.21.3, 1.21.4, 1.21.5)
ERROR: No matching distribution found for numpy>=1.22
Error: Process completed with exit code 1.

@davidhassell
Copy link
Copy Markdown
Collaborator Author

davidhassell commented Feb 9, 2022

Hi Sadie, I think that the choice is between disallowing the latest numpy, or disallowing the oldest Python. I favour the latter. If people really won't/can't drop python 3.7 then they can still use cf-python<v4.0.0 and numpy<1.22.

Also the latest dask is imminently going to drop Python 3.7: dask/community#213

@davidhassell
Copy link
Copy Markdown
Collaborator Author

davidhassell commented Feb 9, 2022

... so I think removing 3.7 from the CI tests would be fine!

@sadielbartholomew
Copy link
Copy Markdown
Member

sadielbartholomew commented Feb 9, 2022

... so I think removing 3.7 from the CI tests would be fine!

Agreed, with that justification 🙂

davidhassell and others added 4 commits February 9, 2022 14:47
@davidhassell @sadielbartholomew
Typo
40244fa 
Co-authored-by: Sadie L. Bartholomew <[email protected]>
@davidhassell @sadielbartholomew
Typo
19b3a7d 
Co-authored-by: Sadie L. Bartholomew <[email protected]>
@davidhassell @sadielbartholomew
Typo
20390aa 
Co-authored-by: Sadie L. Bartholomew <[email protected]>
@davidhassell @sadielbartholomew
Typo
54e3657 
Co-authored-by: Sadie L. Bartholomew <[email protected]>
@sadielbartholomew
Copy link
Copy Markdown
Member

sadielbartholomew commented Feb 9, 2022 •
edited
Loading

(Agreed in a Slack call that I will update the CI workflows to not run on 3.7 in a follow-on PR given we are dropping it as discussed briefly above.)

@sadielbartholomew
Copy link
Copy Markdown
Member

sadielbartholomew commented Feb 9, 2022

Merging, and I will get the workflows updated RE Python 3.7. Thanks again David.

@sadielbartholomew sadielbartholomew merged commit eb5e849 into NCAS-CMS:lama-to-dask Feb 9, 2022
@sadielbartholomew sadielbartholomew mentioned this pull request Feb 9, 2022
Merged
@davidhassell davidhassell deleted the dask-percentile branch November 15, 2022 09:13
@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

@sadielbartholomew sadielbartholomew sadielbartholomew approved these changes

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

@davidhassell @sadielbartholomew