DOC: Add to changes #576
DOC: Add to changes #576
Conversation
|
@agramfort @hoechenberger feel free to push commits if you see the problem. None of these three are trivial to me :( |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
With the change I just pushed and with the changes from this yet-to-be-merged PR for MNE-BIDS, it works for me locally for ds000117: |
…if noise_cov is a callable [circle full]
| @@ -301,7 +301,6 @@ def get_config( | |||
| drop_channels=config.drop_channels, | |||
| find_breaks=config.find_breaks, | |||
| min_break_duration=config.min_break_duration, | |||
| noise_cov=config.noise_cov, | |||
There was a problem hiding this comment.
I think the idea is that this local cfg should have everything that is needed to run and get_input_files, and this breaks that convention.
In terms of following this standard, if we do this (use of config directly) in other cases, we risk breaking the caching because joblib won't see any changes in cfg (I think). In this particular case it will still see the changes I think because the input files will change, for the most part. One case that isn't covered is if it's a callable -- now if the code of that callable changes, the caching won't work. (Actually I suspect it will show up as "always new" because by default the __repr__ of a function includes its id/address, which will change on almost every run even if the code doesn't change. But maybe joblib is smarter about handling this already...?)
For now we could mark this with # TODO FIX just to get things green, though, since this seems like a complex problem with no easy solution, and the callable noise_cov case is rare relative to the others. If we do this, we might want to make it always recompute in the case of a callable noise_cov for safety purposes (i.e., we are not smart enough yet to know if someone changes the code within this function).
There was a problem hiding this comment.
I'm currently on my phone so I cannot look, but I believe we're doing the same thing in a few other places where we accept callables, for example I think for that T1 path config option
But there might be others as well
Finding a clean solution for all of these would be great.
|
@hoechenberger it looks like you missed my comment above (your merge was only 4 min after my comment so understandable!) -- can you take a look? |
|
Okay, thanks for the info and getting this green so quickly @hoechenberger . That would have have taken me quite some time to figure out... |
Pushing with a
circle fullsince we hadn't done it for a while in #563 (whoops!)