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

Changes to implement CFA-0.6 #630

Merged
merged 151 commits into from
Apr 24, 2023
Merged

Changes to implement CFA-0.6 #630

merged 151 commits into from
Apr 24, 2023

Conversation

davidhassell
Copy link
Collaborator

@davidhassell davidhassell commented Apr 4, 2023

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.

davidhassell and others added 2 commits April 24, 2023 10:52
Co-authored-by: Sadie L. Bartholomew <[email protected]>
Co-authored-by: Sadie L. Bartholomew <[email protected]>
@davidhassell davidhassell linked an issue Apr 24, 2023 that may be closed by this pull request
@davidhassell davidhassell added the aggregation Rerlating to metadata-based field and domain aggregation label Apr 24, 2023
@davidhassell
Copy link
Collaborator Author

As for my comment #630 (comment) regarding the means for avoidance of circular imports for the mixin modules, I am still thinking on that. How about we bump it to a new Issue? I think that would be best because any changes made towards it might require a considerable amount of code movement and therefore require a more-involved/thorough re-review. And it doesn't strictly relate to CFA-0.6 support, which is implemented successfully here at this point.

Good idea. Let's make a new issue and not complicate the release of 3.15.0 :)

@davidhassell
Copy link
Collaborator Author

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.

@davidhassell
Copy link
Collaborator Author

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

@sadielbartholomew
Copy link
Member

Thanks David, I will do a final sanity check now.

Copy link
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.

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!

Comment on lines +60 to +61
* Removal of CFA-0.4 functionality (CFA-0.6 will introduced at a later
version).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* 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).

@davidhassell
Copy link
Collaborator Author

I added a couple of basic updates to tutorial.rst: 31a53c4

Some more comprehensive CFA docs will be needed, but I'll do those in another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aggregation Rerlating to metadata-based field and domain aggregation CFA Relating to CFA datasets netCDF read Relating to reading netCDF datasets netCDF write Relating to writing netCDF datasets
Projects
None yet
2 participants