-
Notifications
You must be signed in to change notification settings - Fork 19
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
Changes to implement CFA-0.6 #630
Conversation
Co-authored-by: Sadie L. Bartholomew <[email protected]>
Co-authored-by: Sadie L. Bartholomew <[email protected]>
Co-authored-by: Sadie L. Bartholomew <[email protected]>
Good idea. Let's make a new issue and not complicate the release of 3.15.0 :) |
Co-authored-by: Sadie L. Bartholomew <[email protected]>
Thanks for the UML, Sadie. All looks a labyrinthine when taken as a whole (but good to see it in the round!), but following the relevant strands is good. |
@sadielbartholomew, Thanks for the great review - you really caught loads of stuff. I think everything's resolved, but it'd be great to get your confirmation. |
Thanks David, I will do a final sanity check now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One tiny thing left (see in-line), but all feedback, new and old, has now been addressed. I stand by my previous review overall comment (#630 (review)) RE the overall nature of the PR. The test suite passes for me locally (laptop) (though those pesky 67: RuntimeWarning: invalid value encountered in cast
messages - #625 - are still coming through after this PR, when on initial review I thought they'd conveniently disappeared due to these code changes, so I will take another look to fix them after merge.)
Brilliant, please merge!
* Removal of CFA-0.4 functionality (CFA-0.6 will introduced at a later | ||
version). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Removal of CFA-0.4 functionality (CFA-0.6 will introduced at a later | |
version). | |
* Removal of CFA-0.4 functionality (CFA-0.6 will be introduced at a | |
later version). |
I added a couple of basic updates to Some more comprehensive CFA docs will be needed, but I'll do those in another PR. |
Fixes #637, #451, #475
Needs NCAS-CMS/cfdm#255
Edit 2023-04-17: Notes on changes:
Edit 2023-04-18 More notes on changes - all done, now.
It is now possible for any file array object to support multiple filenames, and for the locations of those multiple filenames to be inspected and edited. E.g.
add_file_location
,file_locations
,cfanetcdf.py
, etc.FullFragmentArray(np.ma.masked)
chunk_locations
functions that is used byCFANetCDFArray
abspath
.mixin2
) ecause there are now multiple files that need there own mixin location, to prevent circular imports.cf.Data
objects.cf.Data
objects to CFA-netCDF files.cfa
keyword that allow the configuration of CFA reads.flip
before, but this precludes the ability to write out the PP/UM data as CFA-netCDF, so we have to re-order in lower level manner. Also tidy up the dask array representation, so that it is consistent with how we build similar dask arrays elsewhere (e.g.GatheredArray
).