[MRG + 2] FIX avoid memory cost when sampling from large parameter grids #4840
Conversation
|
Now MRG. Open for review. |
| # Try the next grid | ||
| ind -= modulo[0] | ||
| else: | ||
| offsets = ind // modulo[1:] % sizes |
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? |
|
|
||
| Returns | ||
| ------- | ||
| params : dict of string to any |
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 |
| for sub_grid in self.param_grid: | ||
| # XXX: could memoize information used here | ||
| if not sub_grid: | ||
| if ind == 0: |
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: