Bnb/dev #107
Bnb/dev #107
Conversation
sup3r/models/multi_step.py
Outdated
| @@ -432,7 +448,78 @@ def load(cls, spatial_model_dirs, temporal_model_dirs, verbose=True): | |||
| return cls(s_models, t_models) | |||
|
|
|||
|
|
|||
| class MultiStepSurfaceMetGan(SpatialThenTemporalGan): | |||
| class SpatialGanThenLinearInterp(SpatialThenTemporalBase): | |||
There was a problem hiding this comment.
Why do we need this? Can't you just load a SpatialThenTemporalGan where the temporal model is a linear model?
There was a problem hiding this comment.
SpatialThenTemporalGan loads models which takes model dirs as inputs to the load method. The linear model doesnt load this way. We could change it to load that way though.
There was a problem hiding this comment.
Yeah it makes a lot more sense to change LinearInterp.load() vs build a whole new multistep model. Load is designed to always load from disc anyway, the current LinearInterp.load() doesnt fit this paradigm (clearly it would be better architecture if it did!). This is a good example where we (me) violated polymorphism and it leads to more needlessly complicated architecture down the road. I hadn't thought of loading the linear model from disc but now that you added the save method it makes total sense to do that.
There was a problem hiding this comment.
Lol yeah I thought about changing the linear load method but didnt want to mess with the models youve already built. Ill change it though.
There was a problem hiding this comment.
oh go for it haha this is a great example where i did something without thinking too hard about it. Also no time lost since i didnt have to train a linear model or anything.
sup3r/qa/utilities.py
Outdated
| return [np.mean(tke_wavenumber_spectrum(u[..., t], v[..., t])) | ||
| for t in range(u.shape[-1])] | ||
| diffs = var / scale | ||
| diff_max = diff_max or np.percentile(np.abs(diffs), 99.9) |
There was a problem hiding this comment.
This is a lot of implicit logic. I think the percentile should be a kwarg and described. I also think this operation should be optional by default. It might make sense for wind but not for any other dataset. Also shouldn't the range arg take care of this? Finally, add docstring for scale param
There was a problem hiding this comment.
Yeah range could accomplish the same thing, so could diff_max, but if I don't know the range I want this is a nice default imo. I'd rather not have to manually specify the range to exclude weird outliers.
There was a problem hiding this comment.
okayyy at least move it to a kwarg and describe it in the docstrings so its not implicit
There was a problem hiding this comment.
haha okay sounds good.
…el. added doc strings to utilities.
More stats class refactoring. Added spatial gan + linear interp multi step model