Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

654 grb::set matrix to matrix broken in mixed domain #290

Merged

Conversation

byjtew
Copy link
Collaborator

@byjtew byjtew commented Feb 5, 2024

Closes #241

  • Removed redundancy in the Compressed_Storage::copyFrom(...) functions. The section in charge of copying the row_index and col_start arrays has been moved to an external local function called copyCoordinatesFrom(...)

  • Merged the cast/no-cast Compressed_Storage::copyFrom(...) functions using std::copy_n rather than a for-loop/memcpy switch

@byjtew byjtew requested a review from anyzelman February 5, 2024 09:53
@byjtew byjtew added this to the v0.8 milestone Feb 5, 2024
@byjtew byjtew self-assigned this Feb 5, 2024
@byjtew byjtew added the bug Something isn't working label Feb 5, 2024
@byjtew byjtew marked this pull request as ready for review February 5, 2024 10:00
include/graphblas/base/io.hpp Outdated Show resolved Hide resolved
include/graphblas/reference/compressed_storage.hpp Outdated Show resolved Hide resolved
include/graphblas/reference/compressed_storage.hpp Outdated Show resolved Hide resolved
include/graphblas/reference/compressed_storage.hpp Outdated Show resolved Hide resolved
include/graphblas/reference/io.hpp Outdated Show resolved Hide resolved
tests/unit/copyMixedDomainsMatrices.cpp Outdated Show resolved Hide resolved
include/graphblas/reference/io.hpp Outdated Show resolved Hide resolved
include/graphblas/reference/compressed_storage.hpp Outdated Show resolved Hide resolved
include/graphblas/reference/compressed_storage.hpp Outdated Show resolved Hide resolved
include/graphblas/reference/compressed_storage.hpp Outdated Show resolved Hide resolved
@byjtew byjtew force-pushed the 654-grb-set-matrix-to-matrix-broken-in-mixed-domain branch from 3525fd7 to c63b55c Compare February 5, 2024 15:17
@byjtew byjtew requested a review from anyzelman February 15, 2024 13:11
anyzelman
anyzelman previously approved these changes Feb 15, 2024
@anyzelman
Copy link
Member

Here is a question though: currently the grb::set( matrix, mask, value ); does not take mask descriptors right?

I think there's good reason for not supporting invert_mask, because that implies a quadratic workload (in the size of the matrices), and seems only useful for cases where the mask matrix that is to be inverted has a number of nonzeroes that approaches the same quadratic bound-- i.e., it turns more into a dense computation that is typically best avoided.

However, we could support non-structural masks that inspect the mask-matrix' actual value, and if it compares false, not copy its coordinate into the output matrix. Since we copy row-by-row this should not be hard to implement. The current semantics would then be activated once more when passing the `descriptors::structural' in there. Would that be hard to add in @byjtew ?

@anyzelman
Copy link
Member

I do note that in disagreement to what I wrote above, that the GraphBLAS C API does support mask inversion even in the matrix case.

@anyzelman
Copy link
Member

Actually now also partially disagreeing with myself: it is still useful to invert a non-structural mask. We could forbid (static_assert) inverting a structural mask specifically maybe?

@byjtew byjtew removed their assignment Feb 27, 2024
@anyzelman
Copy link
Member

This one is ready to merge (after conflict resolution)

@anyzelman anyzelman self-requested a review February 29, 2024 13:07
byjtew and others added 12 commits January 29, 2025 11:06
- Removed redundancy in the Compressed_Storage::copyFrom(...) functions. The section in charge of copying the row_index and col_start arrays has been moved to an external local function.

- Merged the cast/no-cast Compressed_Storage::copyFrom(...) functions using std::copy_n rather than a for-loop/memcpy switch
@anyzelman anyzelman force-pushed the 654-grb-set-matrix-to-matrix-broken-in-mixed-domain branch from eea7653 to 76284d9 Compare January 29, 2025 12:35
@anyzelman
Copy link
Member

anyzelman commented Jan 29, 2025

Kind of feel bad for not merging this earlier - a bunch of conflicts emerged meanwhile, now (hopefully) fixed. Also did a cursory code review and fixes due to the rebase. Running unit tests now to make sure nothing broke, then will continue review. I saw at least one TODO:

  • while I understand the intent of the omp simd pragmas, I don't believe they actually trigger at the moment. Can be re-written using the standard way for "auto"-vectorising applied everywhere else in ALP

…imd vectorisation through OpenMP (of all things that could happen, it surprisingly deadlocked)
…going back to how this MR once started in fact:)
@anyzelman
Copy link
Member

Code review OK, GitHub CI OK. Waiting internal CI OK and manual unit test run with LPF OK before merge.

@anyzelman
Copy link
Member

anyzelman commented Jan 31, 2025

Concept release notes:

This MR fixes the behaviour of grb::set( matrix, * ) by 1) correctly interpreting output masks and 2) fixes the behaviour of casting values from the input matrix value type to that of the output matrix.

This MR also

  • adds a unit test for both fixes to guard against regressions;
  • fixes / updates the base specification of grb::set( matrix, * );
  • allows for grb::set( matrix, matrix[, scalar] ) to work for two matrices with different index / offset types;
  • improves debug tracing for the reference Compressed_Storage class;
  • includes minor code style fixes.

Thanks to Benjamin Lozes for initially identifying the issue and providing the fix!

@anyzelman
Copy link
Member

Internal CI & internal unit tests (with LPF) OK; will merge

@anyzelman anyzelman merged commit d82b61b into develop Feb 1, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

grb::set matrix-to-matrix broken in mixed-domain
2 participants