316 invert mask descriptor inverts return value to true for unassigned elements #317
Conversation
anyzelman
left a comment
anyzelman
left a comment
There was a problem hiding this comment.
Looks almost correct, but also looks like there are some changes to existing unit tests that seem erroneously pushed here and should be reverted prior to merge. Should also scan if one of the changes actually tested for the case raised in the issue.
include/graphblas/utils.hpp
Outdated
| bool ret = assigned; | ||
| // check whether we should return the inverted value | ||
| if( descriptor & descriptors::invert_mask ) { | ||
| if( ( descriptor & descriptors::structural_complement ) == descriptors::structural_complement ) { |
There was a problem hiding this comment.
I think this is almost correct -- mask inversion should also still be in here.
There was a problem hiding this comment.
This is a bit weird, since structural_complement is defined as structural | invert_mask.
Using only the invert_mask descriptor with a void mask can only mean a structural_complement.
So only checking invert_mask should be enough, since structural is the only possible interpretation of a void mask (we have no value)
If this reasoning is correct, I would say that the change should be reverted and we're fine
The changes in the unit tests have all something to do with masks. I think they are wrong tests that expected the old wrong behavior of masking I will try to revert them and run them, just to be sure |
|
I can confirm that the changes in the tests are due to the change in behavior of masking fixed in this commit |
to make sure though - is that with our without the change from structural_complement to invert_mask? |
|
I tested with the original code, so with |
|
The failed CI seems to be an issue with github. Compiles fine locally and on GitLab. I will retry to build tomorrow |
Closes #316