search

Algebraic-Programming / ALP

Home of ALP/GraphBLAS and ALP/Pregel, featuring shared- and distributed-memory auto-parallelisation of linear algebraic and vertex-centric programs. Soon with more to come!
Apache License 2.0
25 stars 4 forks source link

283 provide the implementation of the masked outer product #301

Open aleksamilisavljevic opened 8 months ago

aleksamilisavljevic commented 8 months ago

Closes #283

Depends on #317

byjtew commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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

aleksamilisavljevic commented 7 months ago

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 commented 7 months ago

Running the test in the internal GitLab @aleksamilisavljevic

FAILING TESTS for all backends

anyzelman commented 7 months ago

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 commented 7 months ago

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.