283 provide the implementation of the masked outer product

[aleksamilisavljevic]

Closes #283

Depends on #317

[byjtew]

Nice implementation, the openMP implementation is pretty quick and easy.
Good job 👍

Also, it would be nice to have a (maybe separate) unit test with a user-provided size of the matrix instead of a single predefined one.

[byjtew]

By the way, why do you call the function maskedOuter and not simply outer, since the arguments are different it wouldn't collide with the non-masked implementation

[byjtew]

I also realised that you were only checking against the structure of the mask (coordinates), but never its values (which is tricky when allowing void & non-void)

[aleksamilisavljevic]

I also realised that you were only checking against the structure of the mask (coordinates), but never its values (which is tricky when allowing void & non-void)

Yes, I was actually implementing the structural mask. @byjtew, if the structural descriptor isn't set, is it intended that values evaluate to true iff mask_raw.getValue(...) returns something that evaluates to true?

[byjtew]

I also realised that you were only checking against the structure of the mask (coordinates), but never its values (which is tricky when allowing void & non-void)

Yes, I was actually implementing the structural mask. @byjtew, if the structural descriptor isn't set, is it intended that values evaluate to true iff mask_raw.getValue(...) returns something that evaluates to true?

Not that simple, you should use utils::interpretMask<descr, T> instead, it will interpret the mask value to true or false and handle the void case for you.

[aleksamilisavljevic]

I also realised that you were only checking against the structure of the mask (coordinates), but never its values (which is tricky when allowing void & non-void)

Yes, I was actually implementing the structural mask. @byjtew, if the structural descriptor isn't set, is it intended that values evaluate to true iff mask_raw.getValue(...) returns something that evaluates to true?

Not that simple, you should use utils::interpretMask<descr, T> instead, it will interpret the mask value to true or false and handle the void case for you.

The descriptors should now be properly handled.

[byjtew]

Feel free to mark the MR as "ready" if the tests pass.

[aleksamilisavljevic]

Feel free to mark the MR as "ready" if the tests pass.

Ok, but now I'm wondering if the structural_complement descriptor should be supported at all. I guess that the similar case could be made for forbidding it as in #242

[byjtew]

Running the test in the internal GitLab
@aleksamilisavljevic

---

FAILING TESTS for all backends

• tests/unit/maskedOuter_ndebug_<backend>
• tests/unit/maskedOuter_debug_<backend>

[anyzelman]

Feel free to mark the MR as "ready" if the tests pass.

Ok, but now I'm wondering if the structural_complement descriptor should be supported at all. I guess that the similar case could be made for forbidding it as in #242

I would agree-- structurally inverting any sparse matrix seems never a good idea

[aleksamilisavljevic]

Running the test in the internal GitLab @aleksamilisavljevic

FAILING TESTS for all backends

* `tests/unit/maskedOuter_ndebug_<backend>`

* `tests/unit/maskedOuter_debug_<backend>`

There was a bug in the way the test status was reported. It should be fine now, but I don't have access to the internal GitLab to check it.

Thanks to @aristeidis-mastoras for helping with the debug.

[anyzelman]

This actually seems ready, so I provisionally slot it for v0.8

[GiovaGa]

Currently maskedOuter unit test is failing. I will look into it.

I noticed that this should be merged only after #317 !
Changes in include/graphblas/utils.hpp are due to the implementation of some features from there. Some manual context fixes may be needed