Adding error handling to model traits#120
Conversation
|
Have you considered separating the notions of model and modelling method (or 'trainer')? That would get rid of the 'model not trained' errors at the type level. Trainer::train : training data -> Model This would also help make serialisation clearer when that's implemented - models become just lumps of data, and they're the things that get serialised. Modelling methods/trainer/otherbettername run algorithms to produce models from training data and to produce predictions from data and a model. |
|
@theotherphil thanks for the comment! Someone else recommended this a while ago and at the time I was really excited about it but a couple things prevented me from moving forwards.
Honestly, at this point in time the first point feels like we could work around this cleanly enough. As for the second I think I'd like to try converting an existing model to see what it looks like and how complex the new API feels. Maybe I'll try to get some eyes on it before merging to see how people feel about it. There were some other issues I ran into while trying this before but none were too severe from memory. |
|
Can't we have both alternatives with a wrapper over the |
|
@tafia - Yeah that is certainly an option too. Though I'm not sure I'd like for both options to exist like that. If we make the change it should be to enforce safety with the type system, but this way we end up with the same problem if the API is invoked directly on the wrapper. |
|
@AtheMathmo: your first point sounds more like an argument in favour of splitting the model out. As a user I wouldn't be sure what calling train twice is supposed to do. Before you mentioned retraining I'd have guessed that calling train twice should either panic or retrain from scratch on the new data only. But in some cases it might mean 'train a new model on the data and combine with the old model equivalently to having trained from scratch using all the data seen thus far' (e.g. if fitting a normal to your data you can just do a weighted combination of the two fitted normals), or maybe 'update the model to reflect new data without a full refit' (e.g. just updating the leaf distributions for a random forest, without retraining the node functions), or maybe 'train from scratch using all the new data and all the previous training data we've seen because we've cached it all somewhere'. It should be fairly easy to make these distinct in the type system, but at the cost of more surface complexity. I think offering a wrapper option could lead to a lot of initial confusion for people learning the library. |
|
@theotherphil I agree with your points totally! However, I wasn't very clear with what I meant. I meant that I avoided the change before as I couldn't see a way to handle the code organization if we require training functionality on models that have been trained (like the one you discussed above). We'd like to reuse much of the code in the training algorithm but it seems difficult to organize this effectively. I think it still makes sense to explore this change and we can figure out the online training complications later. |
AtheMathmo
changed the title
|
I'm fairly happy with this PR now. I decided to go with roughly the following criteria:
I'd be happy to receive feedback on places where it seems I'm not following this! |
| return (post_mean, post_var); | ||
| Ok((post_mean, post_var)) | ||
| } else { | ||
| Err(Error::new(ErrorKind::UntrainedModel, "The model has not been trained.")) |
There was a problem hiding this comment.
This is really happening a lot. Can't we have a special constructor or a more advanced ErrorKind that has or not a &'static str message?
There was a problem hiding this comment.
I more or less copied the std::io implementation but I agree that this case doesn't really fit in. I'll see how easy it is to modify - but it seems this particular error will be disappearing if we modify the model traits.
|
@tafia - I added a new constructor for the untrained model errors. We may end up changing the API to remove these completely though. |
|
To be honest while it is better it still doesn't look good. But it doesn't On 9 Sep 2016 12:05 a.m., "James Lucas" notifications@github.com wrote:
|
|
@tafia I feel the same way. But if you have any suggestions on how I can make it better without overhauling the Otherwise I'll merge this in (at least it's not worse than our current panic with train message approach!) |
|
Sorry for not answering. I have a very limited access to internet. I don't On 14 Sep 2016 12:41 p.m., "James Lucas" notifications@github.com wrote:
|
3 participants
This PR adds some error handling to the
SupModelandUnSupModeltraits. It's flagged as WIP as I have a few tweaks planned - but the core content is ready for review and I'd appreciate some feedback.Note that I haven't attempted to cover all possible panics from within the
trainandpredictfunctions. I think it is non-trivial to decide when such things should occur and so I'd prefer to take a lightweight approach for now and add them in as we go (as this will not cause breakage in the api). With that said - if you see something that should be returning anErrorhere please comment so I can address/create an issue.