Fix issue 395 - pspm_con1 #398
Conversation
teddychao
added
the
Completed & Waiting for Review
src/pspm_con1.m
Outdated
|
|
||
| % there are issues if data.stats has NaN and the corresponding conmat | ||
| % also contains 0 | ||
| idx_issue = isnan(prod(data.stats,2)); |
There was a problem hiding this comment.
? I am not sure to understand why you would need prod here.
Would any(isnan(data.stats),2) does the same thing?
There was a problem hiding this comment.
Yes, you solution has a higher readability. I have updated the code in this line 🙂
| conmat = zeros(numel(connames), paramno); | ||
| for c = 1:numel(convec) | ||
| conmat(c,1:numel(convec{c})) = convec{c}; | ||
| end |
There was a problem hiding this comment.
Tip: If all cells of convec are row-vector, I think conmat = cat(1,convec{:}); is similar to your for loop
There was a problem hiding this comment.
This is not written by me. I just checked and I do not think they do have similar features. In the old code convec could be a 2D matrix thus we may need to do for each row..
src/pspm_con1.m
Outdated
| idx_issue = isnan(prod(data.stats,2)); | ||
| idx_valid_issue = transpose(conmat==0) .* idx_issue; | ||
| idx_invalid_issue = transpose(conmat~=0) .* idx_issue; | ||
| if sum(idx_issue) > 0 |
There was a problem hiding this comment.
Yes! I have changed both of this line and the following line to replace sum by any now.
src/pspm_con1.m
Outdated
| idx_valid_issue = transpose(conmat==0) .* idx_issue; | ||
| idx_invalid_issue = transpose(conmat~=0) .* idx_issue; | ||
| if sum(idx_issue) > 0 | ||
| if sum(idx_valid_issue) > 0 && sum(idx_invalid_issue) == 0 |
There was a problem hiding this comment.
Here, the sum are run on matrices, right?
If so, then it is a bit confusing if you run an if on the resulting boolean vector
There was a problem hiding this comment.
The sum here is just used to detect if there are positive values. I have changed it to any as they are really Boolean vectors. In the new version, these operations are done in every line thus idx_valid_issue and idx_invalid_issue are both vectors.
This reverts commit 72fa735.
|
Hi @jbrochar Thanks for discussing with me about this issue today. Could you please try this data (just load it in matlab) and then run the code in this script (maybe just run them in terminal)? I think it can run and it contains two rows of NaNs? thanks.
|
|
@jbrochar Hi Jules, I have updated |
Fixes #395.
Changes proposed in this pull request:
pspm_con1conval.Explanation
Definitions
Therefore, there are 3 situations when there are issue(s)
Among the three situations, B2 and B3 will lead to the same results of conval as
NaN, thus the processing for B2 and B3 is the same.For B1, the valid parts are converted to be 0 for calculating conval.
The logic is therefore as