BUG: Fix beamformer bug with full rank and reg=0. #5934
Conversation
|
Failures are unrelated and will be fixed by #5932 |
britta-wstnr
left a comment
britta-wstnr
left a comment
There was a problem hiding this comment.
Thanks @larsoner for getting at this so quickly!
Apart from my comments regarding the rank > len(Cm) case it looks good to me.
@MadsJensen , would you mind to have a quick look whether this fixes your problems?
| # first eigenvalue that falls outside the signal subspace or the | ||
| # loading factor used during regularization, whichever is largest. | ||
| if rank >= len(Cm): | ||
| if rank > len(Cm): |
There was a problem hiding this comment.
This would now only come in play if the user manually specified a rank higher than the cov matrix dimensions. Can we see such a thing happening / being useful?
There was a problem hiding this comment.
indeed, I'm confused. What does a rank higher than the data dimensionality mean?
| if rank > len(Cm): | ||
| # Covariance matrix is full rank, no noise subspace! | ||
| # Use the loading factor as noise ceiling. | ||
| if loading_factor == 0: |
There was a problem hiding this comment.
The error message should be changed. First, it suggests that it is not possible to compute a noise level with full-rank cov matrix (also see comment in code above). Second, now this would only be reached if the user was specifying the rank manually already, telling them to do the same thing.
If we want to keep this case I suggest something like:
'Cannot compute noise level for NAI computation since specified rank (%s) is higher than covariance matrix dimensions (%s) and no regularization is specified. Try changing rank to equal the covariance matrix dimensions.' % (rank, len(Cm))
There was a problem hiding this comment.
Seems reasonable to me. @wmvanvliet this is your code I think, thoughts?
There was a problem hiding this comment.
It works on the data now.
There was a problem hiding this comment.
how can the rank of the data be higher than the number of dimensions of the data?
There was a problem hiding this comment.
Well, good question. I think it is now a remainder of the code how it was before ( if rank >= len(Cm) ). As I mentioned above, I do not see an immediate use case for it - except the user specifying a rank manually, maybe in a combination with dropping channels during the filter computation.
Do we all agree then that the if statement can go and we go back to how the noise was computed before for any case? In our experience that worked fine, and is in accordance with what Fieldtrip does, too.
|
errors are unrelated. Merging now. thanks @larsoner |
|
Did we want to make the error message more informative (see suggestion in review)? |
|
oups I merged too quickly...
ping @wmvanvliet
|
|
Thinking about it a bit more, I wonder whether this actually should be an error: if the user specifies a rank, but the actual cov matrix dimensions are smaller than the specified rank, shouldn't that just be corrected, maybe throwing a warning? |
|
it would break anyway no?
… |
|
Not if we would correct the rank to equal the dimensions, i.e., correct it down (and inform the user), no? |
|
I would avoid doing magic implicitely
… |
5 participants
Closes #5928.
@britta-wstnr can you see if the fix looks reasonable and it fixes your problem?