Conversation
Co-authored-by: Sadie L. Bartholomew <sadie.bartholomew@ncas.ac.uk>
Co-authored-by: Sadie L. Bartholomew <sadie.bartholomew@ncas.ac.uk>
Co-authored-by: Sadie L. Bartholomew <sadie.bartholomew@ncas.ac.uk>
Co-authored-by: Sadie L. Bartholomew <sadie.bartholomew@ncas.ac.uk>
Co-authored-by: Sadie L. Bartholomew <sadie.bartholomew@ncas.ac.uk>
Co-authored-by: Sadie L. Bartholomew <sadie.bartholomew@ncas.ac.uk>
Absolutely. Done: 1e7885a |
Co-authored-by: Sadie L. Bartholomew <sadie.bartholomew@ncas.ac.uk>
Co-authored-by: Sadie L. Bartholomew <sadie.bartholomew@ncas.ac.uk>
It's not only the refinement level, but also the spatial coverage. I think I'd rather put this in the docs, a la **Performance**
High refinement levels may require the setting of a very large
Dask chunksize, to prevent a possible run-time failure
resulting from an attempt to create an excessive amount of
chunks for the healpix_index coordinates. For instance, with
the default Dask chunksize of 128 MiB, healpix_index
coordinates at refinement level 29 would need ~206 billion
Dask chunks, which is almost certainly more than enough to
cause a crash. In this case, a Dask chunksize of 1 pebibyte
results in only 24576 Dask chunks, a much more manageable
amount::
>>> cf.chunksize()
<CF Constant: 134217728>
>>> f = cf.example_field(12)
>>> g = f.healpix_increase_refinement_level(10, 'intensive')
>>> assert g.coord('healpix_index').data.npartitions == 1
>>> g = f.healpix_increase_refinement_level(15, 'intensive')
>>> assert g.coord('healpix_index').data.npartitions == 816
>>> with cf.chunksize('1 PiB'):
... g = f.healpix_increase_refinement_level(29, 'intensive')
... assert g.coord('healpix_index').data.npartitions == 24576 |
This is sorted: 3a053b5 |
Co-authored-by: Sadie L. Bartholomew <sadie.bartholomew@ncas.ac.uk>
Yes I agree that's a good solution, especially if there's also spatial coverage to consider (didn't think about that somehow but, naturally) - I did note that useful context written under 'Performance' you'd added and it is good to include that elsewhere to increase visibility. Happy for you to get that added in and then that's this point resolved nicely! |
sadielbartholomew
left a comment
There was a problem hiding this comment.
Thanks for addressing all of the feedback - often with clarifications etc. but I've sanity checked any updates that were made to the PR and all seems good, with the full test suite passing.
I think all that is left to do is:
- see #910 (comment) where I've agreed to your good suggestion as to a resolution, so please add in the extra docs;
- fix one newly-introduced typo as per my in-line suggestion;
- see my new response where I have clarified about my concerns on the new optional dependency specification.
Very happy though overall so please merge once the above has been considered! And I'll add a couple of Issues to the tracker relating to the earth radius and Enum threads.
Co-authored-by: Sadie L. Bartholomew <sadie.bartholomew@ncas.ac.uk>
|
I think we're good to go :) |
Fixes #909
Requires NCAS-CMS/cfdm#371