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

Netcdf separate loadsave #4803

Merged
merged 9 commits into from
Sep 28, 2022
Merged

Conversation

pp-mo
Copy link
Member

@pp-mo pp-mo commented Jun 15, 2022

This "just" splits the iris.fileformats.netcdf module into separate sourcefiles for load + save code.
There's very little talk between the two, and this should makes it much easier for devs to find their way around.
Note that I haven't forced the split upon the user : All the public API is exported in the parent module, so everything there is as it was.

TODO: in draft while I test with Cirrus-Ci memory hacks, just to run the tests.

@pp-mo pp-mo marked this pull request as draft June 15, 2022 17:43
@pp-mo
Copy link
Member Author

pp-mo commented Jun 15, 2022

Status update :
doing sort-of OK
various tests to sort, largely Mock issues

@pp-mo
Copy link
Member Author

pp-mo commented Jun 17, 2022

Rebased following #4503 / #4809
Expecting various fixes still needed...

@pp-mo
Copy link
Member Author

pp-mo commented Jun 17, 2022

Rebased onto #4795 branch (which is passing tests), pro-tem, to get it testing with new GHA-based CI.
TODO: rebase again onto main branch, when #4795 is merged

@pp-mo
Copy link
Member Author

pp-mo commented Jun 20, 2022

Rebased onto main, following #4794

@pp-mo pp-mo marked this pull request as ready for review June 20, 2022 17:20
@pp-mo
Copy link
Member Author

pp-mo commented Jun 20, 2022

Hi @trexfeathers I think this is now ready.
No pressure, but ... I hope we can nail this before it needs rebasing again, because it has proved rather awkward for that.

@trexfeathers trexfeathers self-assigned this Jun 23, 2022
Copy link
Contributor

@trexfeathers trexfeathers left a comment

Choose a reason for hiding this comment

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

I think this looks fine, but there are some minor points I don't understand so hopefully you can help me with those.

lib/iris/fileformats/netcdf/loader.py Show resolved Hide resolved
lib/iris/experimental/ugrid/load.py Show resolved Hide resolved
lib/iris/fileformats/netcdf/saver.py Show resolved Hide resolved
lib/iris/fileformats/netcdf/__init__.py Outdated Show resolved Hide resolved
lib/iris/fileformats/netcdf/saver.py Show resolved Hide resolved
@pp-mo
Copy link
Member Author

pp-mo commented Jun 29, 2022

Brilliant thanks @jamesp .
Busy elsewhere but will respond in a bit ...

@pp-mo
Copy link
Member Author

pp-mo commented Jun 29, 2022

## Context : things this can be used for ?
In addition to just "fixing data a bit" on either load or save,
I think this might possibly help with a number of awkward outstanding desired features relating to netcdf :

UPDATE: oops, put this on the wrong issue !

@trexfeathers trexfeathers removed their assignment Jul 5, 2022
@pp-mo
Copy link
Member Author

pp-mo commented Jul 21, 2022

Thanks @trexfeathers .
I think I finally have it working now (after several tries at fixing the import problem!)
Can you please re-review ?

Copy link
Contributor

@trexfeathers trexfeathers left a comment

Choose a reason for hiding this comment

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

Thanks for the responses @pp-mo, and sorry for the delay.

Please could you add a What's New entry under the Internal heading, as this change is important to flag for developers.

Please also shout if merging this now is going to cause any problems with the Split cube-attrs project. Perhaps it's worth the problems 😅

@pp-mo
Copy link
Member Author

pp-mo commented Sep 28, 2022

Please also shout if merging this now is going to cause any problems with the Split cube-attrs project. Perhaps it's worth the problems sweat_smile

Good thought, but should be nothing serious I think -- this has only moved things, not reorganised.

@trexfeathers trexfeathers merged commit d8f8dac into SciTools:main Sep 28, 2022
This was referenced Sep 28, 2022
trexfeathers added a commit to trexfeathers/iris that referenced this pull request Sep 29, 2022
stephenworsley pushed a commit that referenced this pull request Sep 29, 2022
* Enable manual GHA benchmark run with custom first_commit.

* Modified benchmark netcdf import - #4803.
@trexfeathers
Copy link
Contributor

I've been re-running some missed benchmarks. FWIW you would have seen this (predictable I suppose!):

       before           after         ratio
     [e96a0536]       [d8f8dac7]
-         646±5μs          130±1μs     0.20  import_iris.Iris.time_fileformats_netcdf

@pp-mo pp-mo deleted the netcdf_separate_loadsave branch October 12, 2022 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants