-
Notifications
You must be signed in to change notification settings - Fork 8
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha okay sounds good.
…el. added doc strings to utilities.
More stats class refactoring. Added spatial gan + linear interp multi step model