example cleanup, fixing access to properties#23
Conversation
domfournier
left a comment
domfournier
left a comment
There was a problem hiding this comment.
Great, thanks for this. One question below, and we merge this in.
| self.approx_hessian = approx_hessian | ||
| self.non_linear_relationships = non_linear_relationships | ||
| self.gmm = gmm | ||
| self._gmm = copy.deepcopy(gmm) |
There was a problem hiding this comment.
Should we do some checks on what the input is? Hence using the setter that does the copy as well?
There was a problem hiding this comment.
We could, what do you have in mind?
Some thoughts / insights:
Admittedly, this option is not often use. Its main goal is to be able to restart a PGI where the GMM is being learnt (thus the difference between the reference GMM and the GMM model). gmmref is really the most important here.
The reason I do the deepcopy is that (remembering things from several years ago) I was having issues when pointing to the original object, so it was best to have only internal to the regularization (plus, it is potentially getting modified every iteration, something that the GMM class was not originally designed for, thus the creation of a new object that overwrite the previous).
There was a problem hiding this comment.
Yep for sure. I just think we should check for at least the type and shape of the input to make sure the code can fail fast with a nice message if the input doesn't make sense. We should probably do some typing of the return value as well. I don't even know myself what the input/output looks like.
There was a problem hiding this comment.
gmm is an object of class "WeightedGaussianMixture" (or one of its children class). We would need to have a validate method somewhere then.
We can think of something. This is a very quick first pass. I just checked that tests and examples were running. Up to you if you want me to have all in a single PR or if you want to merge this one and I create others as I go.
2 participants
No description provided.