316 invert mask descriptor inverts return value to true for unassigned elements

[aleksamilisavljevic]

Closes #316

[anyzelman]

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.

[GiovaGa]

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.

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

[GiovaGa]

I can confirm that the changes in the tests are due to the change in behavior of masking fixed in this commit

[anyzelman]

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?

[GiovaGa]

I tested with the original code, so with structural_complement. I have now changed to just check invert_mask and I will run the tests on the internal pipeline

[GiovaGa]

The failed CI seems to be an issue with github. Compiles fine locally and on GitLab. I will retry to build tomorrow

[GiovaGa]

Everything should be fine now. Merging develop into this branch was the (right) solution to the failed build here. Therefore I guess many includes from the commits d37c6d1 to 327169c are probably not needed, but they are not wrong since I grepped for uintptr_t and cstdint and compared the matching files.
@anyzelman feel free to revert those changes if you prefer, but beware that there were some merge conflicts!

[GiovaGa]

There was one failing test, now fixed. Maybe it makes sense to change the test to check other more meaningful things (inverting a mask with a single true element might not be very useful)

[GiovaGa]

These tests remind me of my question #402 so as we are working on masks we could clarify the documentation about that too

[anyzelman]

@GiovaGa I wanted to review this, but I see there's a lot of added cstdlib headers. Could you point to a log where the CI failed without it? It's a bit strange e.g. why the CI does not fail on develop if these were indeed required.

(Even if they are required, it would furthermore be better to handle those in a different MR)

[GiovaGa]

@anyzelman
For a failing build see here for example https://github.com/Algebraic-Programming/ALP/actions/runs/19696257418/job/56421790501
But in the end the problem was something else (that I can't recall right now, and was solved with a merge with develop), so I would say that you can revert those commits.
I will open another issue about these missing headers, as it would surely be good to add them sometime

[anyzelman]

OK I resolved my confusion on this one earlier this morning. There is a difference between applying inversion at the end of the mask computation and thus after the structure has been taken into account to compute the non-inverted mask, or to apply the structure at the end of the computation. This MR proposes the latter, which sounds like reasonable behaviour, but the problem is that the former is also a reasonable behaviour (and furthermore is what some of the tests already assume-- and it was the changes to those tests that got me confused).

I checked the GraphBLAS spec, and the old behaviour is what is defined there. We could still propose to add this new behaviour as well, but it seems better to "invent" a new descriptor for such behaviour to remain compliant with GraphBLAS.

Were there any specific use cases that require this new / alternative behaviour?

[anyzelman]

OK I resolved my confusion on this one earlier this morning. There is a difference between applying inversion at the end of the mask computation and thus after the structure has been taken into account to compute the non-inverted mask, or to apply the structure at the end of the computation. This MR proposes the latter, which sounds like reasonable behaviour, but the problem is that the former is also a reasonable behaviour (and furthermore is what some of the tests already assume-- and it was the changes to those tests that got me confused).

I checked the GraphBLAS spec, and the old behaviour is what is defined there. We could still propose to add this new behaviour as well, but it seems better to "invent" a new descriptor for such behaviour to remain compliant with GraphBLAS.

Were there any specific use cases that require this new / alternative behaviour?

proposes the latter*, apologies -- rather big difference