moving common methods from models to abstract #173
Conversation
|
Waiting @JohnGriffiths to approve this |
| self.sc = sc # matrix node_size x node_size structure connectivity | ||
| self.sc_fitted = torch.tensor(sc, dtype=torch.float32) # placeholder | ||
| self.use_fit_gains = use_fit_gains # flag for fitting gains | ||
| self.use_Laplacian = use_Laplacian |
There was a problem hiding this comment.
use_Laplacian is being removed here
I've discussed this with @JohnWangDataAnalyst. We will revisit this in the future. For present purposes I think it makes sense to remove for the sake of having consistent model structure, and also that these features aren't currently used. But this does need to be revisited. Have created issue #174
There was a problem hiding this comment.
This is the argument being removed, but it looks like the use_Laplacian feature is permanently on. It is important that users are aware of this.
| self.sc = sc # matrix node_size x node_size structure connectivity | ||
| self.sc_fitted = torch.tensor(sc, dtype=torch.float32) # placeholder | ||
| self.use_fit_gains = use_fit_gains # flag for fitting gains | ||
| self.use_Laplacian = use_Laplacian | ||
| self.use_Bifurcation = use_Bifurcation |
There was a problem hiding this comment.
@Andrew-Clappison unless you have any objections I will go ahead and merge
There was a problem hiding this comment.
Glad to see a trend towards simplification of the model and objective function. Didn’t look too closely, but here are some additional non-critical things I noticed that can be considered later:
- Argument type checking has been removed
- The RWWEI2 model is no longer a subclass of AbstractNMM
There was a problem hiding this comment.
Ok thanks @Andrew-Clappison for the additional notes. We will refer to this discussion thread moving forward as we get to these points.
Going ahead with merge now.
setModelparams()
untoch andrew' code
I ran through his two examples but not sure the figures are shown... no errors either