State V0#242
Conversation
romanc
left a comment
There was a problem hiding this comment.
This looks awesome 🚀 I have a couple general questions here and then some style/nitpicks inline in code. Sorry for the lengthy review. Even though this is v0, I think it' s worth to start with a good basis.
Is ndsl/quantity a good place to put state.py? I get that states are built from quantities, so it make sense to have them close. Possible alternatives (and their caveats)
ndsl/state/state.py: Mimicking what we seem to have done so far. Seems kind of silly to have a folder for every class.ndsl/data/state.py: Separate package for data handling. The plan would be to eventually movequantity.pyandfield_bundle.pyhere too.ndsl/dsl/state.py: is theStateclass a dsl concept? is "dsl" reserved for gt4py?
How does this new state class relate to ndsl/io.py? Feels like io.py as built to have a shared ground for saving / loading model state to / from netCDF files. Should we deprecate ndsl/io.py in favor of this state object?
A question about initialization: when initializing to zeros, users get a class method and initialize in one go
test_state = TestState.zeros(quantity_factory)that's what I would expect. In contrast, when initializing from memory and/or netCDF, users have to do a "two stage initialization procedure", initializing everything to zero first and then setting values from memory / disk
test_state = TestState.zeros(quantity_factory)
test_state.from_netcdf("state_dump.nc4")Are there plans to allow TestState.empty(quantity_factory) to avoid first setting all values to 0 and then immediately overriding them? Or would it make sense to change from_netcdf() to a class function, e.g.
test_state = TestState.from_netcdf("state_dump.nc4")?
This will happen as wider clean up of the |
romanc
left a comment
There was a problem hiding this comment.
This is looking nice now 🚀 Couple of nitpicks / questions (about naming again - sorry 🙄) inline. Feel free to dismiss or accept as you see fit.
A first version of the NDSL base class for
State. The class is made to provide a maximum of useful functions for a nesteddataclassofQuantities. This covers the use case of GFDL 1M microphysics in GEOS.This version 0 can (see utest)
I expect this to be a base for further work distributed among users.
Other points:
BoundedArrayViewdacitefor automatic matching of dictionary and nesteddataclasses