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

fix: avoid writing zero via sparse matrix iterator #1989

Merged
merged 2 commits into from Aug 29, 2019

Conversation

@conradsnicta
Copy link
Contributor

commented Aug 28, 2019

Writing a zero to a sparse matrix causes the element to be deleted. The zero is then immediately overwritten by a non-zero value. When using iterators, this is horribly inefficient: memory re-allocation + copying. It also has the potential to screw up the validity of the sparse matrix iterators.

Copy link
Member

left a comment

Nice catch! 👍

@rcurtin rcurtin added c: methods and removed s: unanswered labels Aug 29, 2019
@rcurtin

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

I can handle the style issues during merge and update HISTORY.md. Also, the AppVeyor build failure seems to just be a timeout so I don't think we need to worry about it.

@zoq
zoq approved these changes Aug 29, 2019
Copy link
Member

left a comment

Looks good to me.

@zoq zoq removed the s: needs review label Aug 29, 2019
@rcurtin rcurtin merged commit 2320488 into mlpack:master Aug 29, 2019
4 of 6 checks passed
4 of 6 checks passed
Style Checks Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
LaTeX Documentation Checks Build finished.
Details
Memory Checks Build finished.
Details
Static Code Analysis Checks Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rcurtin

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

Thanks! 👍 If you find anything else do feel free to open more PRs. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.