Skip to content

Conversation

@byjtew
Copy link
Collaborator

@byjtew byjtew commented Dec 1, 2023

Closes #270

  • Wrapping the selection operators in their own namespace to avoid confusion: grb::operators::select::

  • Implement selectLambda

  • Distributed backends implementation

  • In-place variants (should come in a separate PR)

  • Masked variants (should come in a separate PR)

  • Pattern matrices support (should come in a separate PR)

@byjtew byjtew added the enhancement New feature or request label Dec 1, 2023
@byjtew byjtew self-assigned this Dec 1, 2023
@byjtew byjtew force-pushed the 635-provide-grb-select-out-matrix-in-matrix-lambda-bool branch from 37ef7a4 to bcdf77a Compare January 17, 2024 19:08
@byjtew byjtew changed the title Draft of what a selection operator would look like What a selection operator could look like Jan 18, 2024
@byjtew byjtew marked this pull request as ready for review January 18, 2024 09:42
@anyzelman
Copy link
Member

The current state does not have bsp1d nor hybrid implementations of select, that's correct right? I think this is a super useful enhancement, but before merge it should have those distributed-memory implementations also

@byjtew
Copy link
Collaborator Author

byjtew commented Jan 25, 2024

The current state does not have bsp1d nor hybrid implementations of select, that's correct right? I think this is a super useful enhancement, but before merge it should have those distributed-memory implementations also

Absolutely, I also wanted to switch the namespace from operators:: to select_operators:: or operators::select:: since they have different signatures than usual operators::

I'll ping you once it will be implemented

@byjtew byjtew marked this pull request as draft January 25, 2024 15:23
@anyzelman
Copy link
Member

The current state does not have bsp1d nor hybrid implementations of select, that's correct right? I think this is a super useful enhancement, but before merge it should have those distributed-memory implementations also

Absolutely, I also wanted to switch the namespace from operators:: to select_operators:: or operators::select:: since they have different signatures than usual operators::

I'll ping you once it will be implemented

maybe just grb::select?

@byjtew
Copy link
Collaborator Author

byjtew commented Jan 25, 2024

maybe just grb::select?

But then it could be either the primitive or the operator's namespace, what about grb::select::operators ?

@anyzelman
Copy link
Member

maybe just grb::select?

But then it could be either the primitive or the operator's namespace, what about grb::select::operators ?

Oh right.. It could still work by having select as a class with the "primitive call" via operator() and the pre-defined lambdas as public static members, but that's perhaps a bit hacky. Maybe just think of something and we can look at it in more detail later, once it's more or less ready?

@anyzelman anyzelman added this to the v0.9 milestone Jan 29, 2024
@anyzelman
Copy link
Member

Also doc/spec should be added to base/blas3.hpp

@anyzelman anyzelman modified the milestones: v0.9, v0.8 Jan 29, 2024
@byjtew byjtew force-pushed the 635-provide-grb-select-out-matrix-in-matrix-lambda-bool branch from 6c2b2d6 to cbf8519 Compare January 30, 2024 08:06
@byjtew byjtew marked this pull request as ready for review January 30, 2024 09:44
Copy link
Member

@anyzelman anyzelman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this useful primitive!

I think this one can be simplified as in the comments. The suggested simplifications should benefit both usage as well asimplementation.

@byjtew byjtew force-pushed the 635-provide-grb-select-out-matrix-in-matrix-lambda-bool branch from 8c3e082 to 3fa8309 Compare January 31, 2024 16:04
Implementation of:
- CCS update of the output matrix
- Handle transpose_matrix descriptor
- Handle force_row_major descriptor
- Extend the unit-test
- Add unit-test to the testing pipeline
- selection operator: is_strictly_lower
- selection operator: is_strictly_upper
- selection operator: is_lower_or_diagonal
- selection operator: is_upper_or_diagonal
anyzelman and others added 6 commits February 28, 2024 16:30
…::select, thus removing the need for selectLambda. Tests pass, except for BSP1D. Minimal failure (as far as known) occurs test size 65, P=4
@byjtew byjtew requested review from anyzelman and removed request for aleksamilisavljevic February 29, 2024 09:36
@anyzelman
Copy link
Member

This one is ready to merge (after conflict resolution)

@anyzelman
Copy link
Member

This one is ready to merge (after conflict resolution)

Correction: unit tests still detect a bug in BSP1D, fixing it is TODO

@anyzelman
Copy link
Member

Fixed

@anyzelman
Copy link
Member

anyzelman commented Mar 2, 2024

Some final tests (with LPF) are running, as is the CI. Will merge if all OK.

Concept release notes:

This MR introduces the matrix selection feature -- given a operator that takes matrix nonzero coordinates and a value as arguments and produces a boolean, elements from an input matrix are selected to be copied in some output matrix. This primitive operates in an out-of-place fashion. In-place semantics may be achieved via a temporary (pattern) matrix and fold, and thus are currently not planned to be added. Standard matrix selection operators such as (strictly) lower triangular etc. are provided, including type traits for them. Non-standard selection operators provided by users are simple to add as the new grb::select primitive accepts arbitrary lambda functions as a selection operator as well.

This MR also fixes a bug for the BSP1D and hybrid eWiseApply on matrices with the resize mode, which could return SUCCESS even if it was not successful. The MR includes unit tests for matrix distributions and the new select operator, as well as code style fixes in related and unrelated files.

@anyzelman anyzelman merged commit 9cb0393 into develop Mar 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Provide grb::select( [out] Matrix, [in] Matrix, grb::operator(r,c,v) -> bool )

3 participants