-
Notifications
You must be signed in to change notification settings - Fork 4
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
654 grb::set matrix to matrix broken in mixed domain #290
Conversation
3525fd7
to
c63b55c
Compare
Here is a question though: currently the I think there's good reason for not supporting 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 ? |
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. |
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? |
This one is ready to merge (after conflict resolution) |
- 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
eea7653
to
76284d9
Compare
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:
|
…imd vectorisation through OpenMP (of all things that could happen, it surprisingly deadlocked)
…going back to how this MR once started in fact:)
Code review OK, GitHub CI OK. Waiting internal CI OK and manual unit test run with LPF OK before merge. |
Concept release notes: This MR fixes the behaviour of This MR also
Thanks to Benjamin Lozes for initially identifying the issue and providing the fix! |
Internal CI & internal unit tests (with LPF) OK; will merge |
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 calledcopyCoordinatesFrom(...)
Merged the cast/no-cast
Compressed_Storage::copyFrom(...)
functions usingstd::copy_n
rather than a for-loop/memcpy switch