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

[Sparse] Add clear_triplets() #1141

Merged
merged 7 commits into from Aug 22, 2022
Merged

Conversation

lsh
Copy link
Contributor

@lsh lsh commented Aug 16, 2022

This PR adds the clear_triplet() method to the CooMatrix struct.
The goal of the method is to be able to remove triplets before construction.

Some outstanding questions:

  • Spelling of the method? @Andlon suggested clear_triplet in the Discord. Another option might be pop or pop_triplet to correspond to push.
  • Should out of bounds values cause a panic? It is probably fine to just return a None value.
  • What should the return type be? Currently it is Option<T>.

@Andlon
Copy link
Sponsor Collaborator

Andlon commented Aug 16, 2022

Thanks for working on this @lsh!

Actually, I suggested clear_triplets (note plural), for removing all triplets. This is very useful as it would allow us to re-use already allocated COO storage and therefore amortize allocations. I'm not so sure if removing a single triplet based on coordinates is very useful as it is terribly inefficient - do we have a compelling motivating use case for this?

@lsh
Copy link
Contributor Author

lsh commented Aug 16, 2022

@Andlon whoops, sorry about my misunderstanding 😅. Updated the code to implement clear_triplets.

@lsh lsh changed the title [Sparse] Add clear_triplet() [Sparse] Add clear_triplets() Aug 16, 2022
@Andlon
Copy link
Sponsor Collaborator

Andlon commented Aug 16, 2022

@lsh: great, thanks. CI is failing because you have the wrong return type for clear_triplets, maybe try running tests locally?

Copy link
Sponsor Collaborator

@Andlon Andlon left a comment

Choose a reason for hiding this comment

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

@lsh: just one unnecessary trait bound that needs to be removed, otherwise looks good! Tests are still running but assuming they succeed after removing the trait bound we should be good to go.

nalgebra-sparse/src/coo.rs Outdated Show resolved Hide resolved
@Andlon
Copy link
Sponsor Collaborator

Andlon commented Aug 19, 2022

@sebcrozet: not sure why the CUDA build is failing. Do you? Looks like some strange issues with the recent changes that made Point::new a const fn?

@sebcrozet
Copy link
Member

sebcrozet commented Aug 19, 2022

@Andlon You’re right, looks like #1112 broke the CUDA build (and for some reasons the CI didn’t run of that PR). I think this is due to the fact that the CUDA build uses an older version of the compiler which only allowed Sized trait bounds for const fn. We should add a non-const version when targeting cuda.

@lsh lsh requested a review from Andlon August 20, 2022 07:10
@Andlon
Copy link
Sponsor Collaborator

Andlon commented Aug 22, 2022

Looks good to me, ready for merge. @sebcrozet: do we wait with merging until the CUDA CI problems are fixed, or what's your preference?

@sebcrozet
Copy link
Member

Thank you both! No need to delay this on something unrelated.

@sebcrozet sebcrozet merged commit 5cf6afb into dimforge:dev Aug 22, 2022
@lsh lsh deleted the sparse-clear-triplet branch August 22, 2022 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants