Closed flukeskywalker closed 9 years ago
For Issue A, I'd like to integrate some/most of the sklearn score functions. That should be rather straight-forward, and would give us lots of nice monitors. I'll have to check whether we can just write a general MonitorScore
that takes any sklearn score function. If that works I would strongly suggest we do that (i.e. option 2).
Regarding Issue B: The monitors themselves I wouldn't rename, only the mechanism. So you would maybe do: network.add_hook(MonitorScore(...))
.
If we later have "Monitors" that do more than just monitoring we can call them "...Hook".
I support utilizing sklearn score functions. This would add sklearn as a dependency, but it makes sense. Perhaps we can bring in more goodies from sklearn such as text preprocessing etc.
On B, it might get a little confusing: We'd say that you can add both Monitors and Hooks, but Monitors are actually hooks, so you add them both using add_hook() :-S
Regarding B:
I also think "Monitor" is slightly confusing as a term. Who would guess that trainer.add_monitor(bs.MaxEpochsSeen(500))
is actually the correct way to set an upper limit on the number of epochs? If 'hook' does not fit the bill, then maybe add_post_epoch_callback
? (though I like hook more than that... but I'm sure there are even better-fitting names).
MaxEpochsSeen
is a good example of a Monitor
that is not a monitor. We really should rename them. I do like add_hook
better, although add_callback
is also a possibility (I would skip the post_epoch
part, also because they can be called update-wise).
Now the next question becomes: Do we enforce a naming convention for hooks/callbacks? So would we use for example MaxEpochsSeenHook
or MonitorScoreHook
or do we leave them without the Hook
suffix?
I'd just not import them into the current namespace: hooks.MaxEpochsSeen
Keeping the suffix would be consistent with layers, trainers, steps etc. Things like Iterators, weight modifiers etc. currently do not follow this convention.
On 17 August 2015 at 22:38, Klaus Greff notifications@github.com wrote:
MaxEpochsSeen is a good example of a Monitor that is not a monitor. We really should rename them. I do like add_hook better, although add_callback is also a possibility (I would skip the post_epoch part, also because they can be called update-wise).
Now the next question becomes: Do we enforce a naming convention for hooks/callbacks? So would we use for example MaxEpochsSeenHook or MonitorScoreHook or do we leave them without the Hook suffix?
— Reply to this email directly or view it on GitHub https://github.com/Qwlouse/brainstorm/issues/26#issuecomment-131955146.
Perhaps we should remove the suffix convention altogether from the library? Perhaps it is not of much value and will make scripts look better and less verbose.
After reading @untom's suggestion I was thinking the same. Maybe we should have bs.layers.Rnn
instead of bs.RnnLayer
and so on. I think I would like that.
I took care of the renaming. See if you like it:
bs.layers
namespaceLayerImpl
_layer.py
hooks
in the Trainer
bs.hooks
namespaceMonitor
prefix from MonitorLoss
and so onLGTM
Issue A
Proposals:
2 will prevent code duplication. 1 might be more intuitive to use?
Issue B
Additionally, me and @Qwlouse talked some time ago about perhaps renaming Monitors (perhaps call them 'Hooks'?) This is because Monitors can do more than just monitoring stuff.