Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Remove auto-cache mechanism #1780
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
Remove auto-cache mechanism #1780
Changes from all commits
eae2947
7294b6e
426514c
e368a1a
a17e3fe
369bf11
47d213e
dd8f3c3
fc8285c
504f148
5e08d1e
176b7c1
067c88d
1d963bd
bca4ccf
1342475
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
As requested by @GalOshri in the issue, can we add documentation?
Currently the user will have no method of knowing if a specific learner already does it own form of caching, or won't benefit from caching.
Inline w/ @GalOshri's request, I think this documentation should be required before making this change.
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.
Let's change the appropriate cookbook samples to illustrate the new pattern with this little caching checkpoint thing.
In reply to: 237951473 [](ancestors = 237951473)
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.
Yes, updating the example code is a good first step. And we should create a direct list of the components which benefit from caching. This is along with when they benefit, for instance, "a LinearSVM when the number of iterations are greater than 1".
Another route is perhaps a VS checker which look at Info.WantCaching and recommends from there? #WontFix
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.
A sample and some tests are modified to use those caching functions. Every caching function has at least one test now. #Resolved
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 don't think having a list is a small task. We need another PR and issue.
In reply to: 237953879 [](ancestors = 237953879)
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.
Ok. I will do it in next iteration.
[Update] Done. Please take a look again. Thank you.
In reply to: 237951764 [](ancestors = 237951764,237951473)
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.
Generally, I like to see documentation in the PR. This is more so true when the user can be surprised by the change and not understand what's different.
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.
Many uses are added into Coookbook.
In reply to: 238949665 [](ancestors = 238949665)