-
Notifications
You must be signed in to change notification settings - Fork 75
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
Rename TemporalMemory extra to externalPredictiveInputs #519
Conversation
There was confusion about what this feature does so I renamed it for clarity.
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.
Looks good, thanks!
Consider some more details for the description..?
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.
Improved doc and clearer code is always the best!
Thanks
@@ -99,6 +99,11 @@ class TemporalMemory : public Serializable | |||
* @param predictedSegmentDecrement | |||
* Amount by which segments are punished for incorrect predictions. | |||
* | |||
* Note: A good value is just a bit larger than | |||
* (the column-level sparsity * permanenceIncrement). So, if column-level |
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.
OT: #279 it would be nice to reduce the number of parameters for the algorithms, especially in cases like this where a good-enough value can be derived from other settings. Do you think this would be a reasonable candidate to hard-code?
Pros:
- less space for user to mess up
- faster parm optimizations
Cons: - removes a bit of configurability
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.
I agree, but here are two more potential issues:
- Is this advice always good? I think it would need further investigation before we remove the parameter in favor of this heuristic.
- We would have to modify every instance of the TM to omit this parameter. It would be a lot of work.
There was confusion about what this feature does so I renamed it
for clarity.
See also issue #512.