-
Notifications
You must be signed in to change notification settings - Fork 269
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
Setting prefix for composite models #121
Comments
I could be wrong (and writing from a phone, away from computer) but the prefix might be useful for added params as from param_hints. For example, you could set a midpoint between two Gaussian centers or similar. I think that might make the composite prefix worth keeping, though not vital. |
Sorry, can you give an example? |
How about:
One interesting side effect is that if one then adds another component:
that hinted parameter Yes, one could fully name the hinted-into-existence parameters, but I don't see why the prefix approach should be disallowed. |
I'm not sure I follow that perfectly. What does |
It gives:
so that |
I wrote:
and I was wrong. We can end up with composite models containing composite models:
It maybe even work when fitting but I find it confusing. I think we should stick with one level of composition. And returning to Matt example, it works because one model is composite and is actually extended by adding a new base-model component. So the hints and prefix of the composite model are preserved. But if we allow composing (adding) two composite models we should:
Points 1 and 2 are easy to implement. But how do we handle point 3? |
Food for thoughts: we should probably think of treating prefix and hints with the same logic. However hints can never conflict because the parameters are always disjoints. |
I don't see a fundamental problem with nested models (and I think they ought to work), but adding a check in Should point 3 read "handle the case with two composite models have the same prefix"? (If not, I don't see a problem). If so, I'm OK with leaving that as "allowed but probably not what you wanted to do". My guess is that we'll hear many more reports of unintended usages after release than we can ever come up with ourselves. I also think that's OK. The Model class is kind of a big addition so I think it is OK to expect that we haven't gotten all aspects perfect. |
how so? or What logic is that?
Hints are applied to parameters, possibly implying the creation of new parameters. I think hints might be able to conflict with one another (if you worked at it), but so what? |
Prefix on composite models are only used for parameters from hints. The name of those parameters is compute on-fly using
So it seems to me that the easy case is when the two prefixes are the same. In that case, you are right, we could have a conflicting hint on a hint-defined parameter (not on the other parameters). But this can handled I guess by dropping one hint and issuing a warning. I don't know how to handle the case of different prefixes (either than dropping one and warning the user). |
Concrete examples might help. Without clear examples of obvious problems, I'm -0.5 (willing to be out-voted) on adding much code to deal with such what-if cases. When building composites or copying models, there may be some non-obvious behavior for parameter hints, but there will be some behavior, and one can always alter the hints after the fact, or alter the parameters for that matter. If the behavior with the current code is clearly wrong it should be fixed, otherwise, probably not. Trying to predict and handle every corner case is not worth the effort. |
Ok, I see your point. I made a PR (#122) that only flattens the components and merges the hints. There is this corner case of two composite model with different prefixes, but it can be handled later if it really hurts somebody. |
is this issue close-able? |
Yes. |
We recently changed
Model.prefix
to a property.In composite models we allow setting the prefix but we never use it:
I think the prefix setter should raise a warning and not set the prefix for composite models. Setting the prefix makes sense only for base models in current implementation IMHO. We have only base models and composite models. We don't have composite models made of composite models (the components are always split up to the base model). I think this approach is good. But makes composite models prefix of little use (if any).
Comments?
The text was updated successfully, but these errors were encountered: