-
Notifications
You must be signed in to change notification settings - Fork 18
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
Enable in-memory feature caching for properties #472
base: master
Are you sure you want to change the base?
Conversation
One big concern I have is the confusion that it might create a little confusion with |
val a = new BooleanProperty[T](name, cachedF) with NodeProperty[T] { override def node: Node[T] = papply.node } | ||
val a = new BooleanProperty[T](name, cachedF) with NodeProperty[T] { | ||
override def node: Node[T] = papply.node | ||
override val isCachingEnabled: Boolean = isStatic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe change isCachingEnabled
to sth closed to static
? isStatic
(or isStaticEnabled
) itself sounds more clear
On the implementation, I'd suggestion different approach: we can reuse the existing caching. The only thing we have to do is to make sure we don't clean in between each training iterations. That is handled here by this function.. So the only change needed is, not calling the |
c17096e
to
699f238
Compare
ce9450a
to
08dec8b
Compare
This is ready to be reviewed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one renaming suggestions, otherwise this is good to me to merge.
If you want to cache the value of a feature during a single iteration, use the `cache` parameter. | ||
|
||
The `cache` parameter allows the value to be cached within a training/testing iteration. This is useful if you one of your features depends on evaluation of a Classifier on other instances as well. This recursive evaluation of the Classifier might be expensive and caching would speed-up performance. Look at a sample usage of this parameter in the [POSTagging Example](../../saul-examples/src/main/scala/edu/illinois/cs/cogcomp/saulexamples/nlp/POSTagger/POSDataModel.scala#L66). | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you one of your => if one of your
val posWindow = property(token, cache = true) { | ||
(t: ConllRawToken) => t.getNeighbors.map(n => posWindow(n)) | ||
} | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe iterationCache
or perIterationCache
? (instead of cache)
Work Items
Verify/Reason if we need a function to explicitly clear the cache?Idea: Cache static features that do not change across learning iterations. Improves training speed at the cost of using more memory for caching the features. Tested this on the Chunker app and there is a significant improvements to training time.