typing: add types to ndsl.stencils.testing.grid#404
Conversation
These functions of the `Grid` class must be dead code because the `Quantity` constructor now requires a backend argument (which isn't provided). If anyone were to use that code, we'd see a type error.
Add type hints to the `Grid` class used in translate tests. This is not perfect, but at least we have now most types. Lots of magic happening here and if we don't have minimal type hint and type checking we'll missing things like the `Quantity` update in the previous commit.
| def make_quantity( | ||
| self, | ||
| array, | ||
| dims=(I_DIM, J_DIM, K_DIM), | ||
| units="Unknown", | ||
| origin=None, | ||
| extent=None, | ||
| ): | ||
| if origin is None: | ||
| origin = self.compute_origin() | ||
| if extent is None: | ||
| extent = self.domain_shape_compute() | ||
| return Quantity(array, dims=dims, units=units, origin=origin, extent=extent) | ||
|
|
||
| def quantity_dict_update( | ||
| self, | ||
| data_dict, | ||
| varname, | ||
| dims=(I_DIM, J_DIM, K_DIM), | ||
| units="Unknown", | ||
| ): | ||
| data_dict[varname + "_quantity"] = self.quantity_wrap( | ||
| data_dict[varname], dims=dims, units=units | ||
| ) | ||
|
|
||
| def quantity_wrap( | ||
| self, | ||
| data, | ||
| dims=(I_DIM, J_DIM, K_DIM), | ||
| units="unknown", | ||
| ): | ||
| origin = self.sizer.get_origin(dims) | ||
| extent = self.sizer.get_extent(dims) | ||
| return Quantity(data, dims=dims, units=units, origin=origin, extent=extent) |
There was a problem hiding this comment.
Must be dead code because the Quantity constructor has a required backend argument. If we were still using these code paths, they'd be getting TypeErrors.
There was a problem hiding this comment.
Dead code for sure. Probably lined to translate system at some point to move toward a full quantity.
There was a problem hiding this comment.
🧹
Having a stencils.testing.grid has been a vexing issue for a while but I am yet to take the time to nuke this monstrosity from orbit. I am yet to understand why we can't centralize the concept of grid and have this thing kicking around... Work for another time (sic).
|
yeah, I had a quick look why this class exists in the first place and very quickly stopped looking 🙈 Indeed it seems this has to be redundant (or at least just for convenience), but it seems to pull in a couple of concepts that I'm not yet super familiar with, so I just left it there. To be revisited later(tm) |
2 participants
Description
Add type hints to the
Gridclass used in translate tests. This is not perfect, but at least we have now most types. Lots of magic happening here and if we don't have minimal type hint and type checking we'll missing things like theQuantityupdate in the first commit.How has this been tested?
All good as long as CI is still green. Locally,
mypyis happy.Checklist