Types take 2#162
Conversation
| if "siglay" in ds.dims: | ||
| siglay = ds["siglay"].isel(node=0) | ||
| elevation_index = int(np.absolute(siglay - level).argmin(axis=0).values) | ||
| elevation_index = int(np.absolute((siglay - level).to_numpy()).argmin(axis=0)) |
There was a problem hiding this comment.
This is a nice mypy "find", by converting to a numpy array early, the two numpy methods we use (absolute and argmin) are always applied to a numpy array and avoid surprises, like when a method has the same name, but different behaviour in similar objects. For example the dataframe .std vs the numpy .std methods.
There was a problem hiding this comment.
Doesn't np.absolute(arrray_like) always return a numpy array anyway?
|
|
||
| # Merge the subsetted datasets | ||
| ds_out = xr.merge(ds_out) | ||
| _ds_out = xr.merge(ds_out) |
There was a problem hiding this comment.
These happen b/c we are re-using variable names. It is harmless, but I like the readability of easily knowing that the variable changed its type and b/c of that deserves a new name.
There was a problem hiding this comment.
hmm -- does xr.merge() change the data type? Isn't it a dataset before an after?
In this code, there's quite a lot of processing of a Dataset step by step. -- each step usually creating a smaller Dataset -- so there is something to be said for re-using the same name so the old one will be immediately deleted.
The actual data is probably lazy-loaded, so maybe it doesn't matter, but I think it's kind of clean.
If we are going to use a new name at each step, maybe a meaningful one:
ds_merged = xr.merge(ds_out)
(I don't know if that's a good name, I haven't looked to see what this actually doing)
Or even ds_1, ds_2, ....
|
pre-commits is failing with: Looks like we will need to run mypy on GitHub Actions instead. I'll do it in this PR tomorrow. |
2 participants
This PR fix the remaining type issues for a full realized env with all the dependencies. There is not much we can do for
cftimeb/c there are no stubs there, and we need to make a call on the genericHashabletypes for the xarray datavars, dims, etc. See pydata/xarray#4834. Or options are:Hashableand wait for a better fix upstream;xarrayfrom the mypy env and add--ignore-missing-imports;typing.castand cast them tostr.I prefer option to ignore them for now b/c it a way to explicitly remind us to revisit this in the future.