[MRG + 2] FIX avoid memory cost when sampling from large parameter grids#4840
[MRG + 2] FIX avoid memory cost when sampling from large parameter grids#4840jnothman merged 1 commit intoscikit-learn:masterfrom
Conversation
|
Now MRG. Open for review. |
sklearn/grid_search.py
Outdated
There was a problem hiding this comment.
not sure if there is a helpful comment that can be done on this line. took me a minute but alright.
There was a problem hiding this comment.
Well yes, I guess the code is unnecessarily obfuscating seeing as the numbers are small so efficiency is little concern. I'd initially thought I'd memoize the modulo arrays, but that seems overkill. I'll rewrite more legibly.
There was a problem hiding this comment.
Please give this another quick look to see if it needs further clarification.
|
Looks good apart from the fact that I'm not 100% sure the tests is non-vacuous. |
|
thanks for the fix :) |
|
lgtm. |
|
great, thanks! On 10 June 2015 at 07:10, Andreas Mueller notifications@github.com wrote:
|
|
@jnothman super cool 👍 Thanks for the quick fix!, nice and elegant |
|
Another review? |
There was a problem hiding this comment.
The same description is used above in object constructor. It means "a dict mapping strings to arbitrary-type values".
|
looks good |
There was a problem hiding this comment.
Is this branch covered by the test? It looks to me like the test doesn't cover subgrids.
Nevermind, somehow I missed that line. But I still would like a test where a non-empty subgrid follows an empty subgrid, for the else below.
|
@vene mrg? |
|
Looks good to me 👍 |
[MRG] FIX avoid memory cost when sampling from large parameter grids
|
Thanks, @chausler for reporting, and to the reviewers! |
Solve the issue reported at #3850 (comment) by making
ParameterGridable to dynamically look up a param setting by index; i.e. list of all possible settings is never realised.Needs: