Repository navigation
Add copy and deepcopy for KLUFactorization - #45
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #45 +/- ##
==========================================
+ Coverage 59.02% 61.86% +2.84%
==========================================
Files 2 2
Lines 593 611 +18
==========================================
+ Hits 350 378 +28
+ Misses 243 233 -10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This was referenced Oct 30, 2026
ViralBShah
force-pushed
the
copy-klufactorization
branch
from
October 30, 2026 19:30
ea98809 to
0fdca0e
Compare
The default deepcopy copied the raw pointers to the libklu objects, so a copy shared its factors and scratch space with its source and had no finalizer. Refactoring one changed the other, and freeing one left the other with dangling pointers, leading to wrong answers, segfaults, and double frees. libklu can't duplicate its objects, so a copy is analyzed and factored again from the current values and settings, and stays in the same state as its source. Fixes #12 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ViralBShah
force-pushed
the
copy-klufactorization
branch
from
October 30, 2026 19:59
0fdca0e to
7a0b485
Compare
The copied common carried over memusage and mempeak from the source, so a copy reported twice the memory it used and never returned to 0 after freeing. Fold the 266 lines of tests into the existing testsets, keeping one assertion per behaviour. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Member
Author
|
Review fixes in 50ce661. A copy carried over |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #12.
The bug
KLUFactorizationholds raw pointers to its libklu objects and had nodeepcopymethod, so the default copied the addresses. A copy therefore shared its factors and scratch space with the source, and it had no finalizer of its own.copywas aMethodError.Results at
main, each scenario in its own process:klu!(the case in the issue)SingularExceptionon 3 of 4 tasksklu_factor!(copy), then a solve on the sourceklu_solveklu_factor!(copy)The "After" column was measured on an earlier revision that also tried to recover the factored values when
nonzeros(K)had been modified. That logic was removed before opening this PR. The final code was checked with the test suite, which covers the same independence, freeing and concurrentklu!cases, rather than by repeating these runs.Behaviour
copyanddeepcopyboth return a fully independent object with its own arrays,common, lock and libklu objects. libklu has no function for duplicating them, so an analyzed or factored source is analyzed and factored again, at the cost of a new factorization.commonis copied as it stands at the time of the copy. Permutations fromklu_analyze!(K, P, Q)are reused.nonzeros(K)changed afterKwas factored, the copy factors the current values, so it differs from its source until the source is refactored.klu!: the copy chooses its own pivots, so its factors can differ from the source's, but both solve the same system.copyis deep too:klu!(K, S)temporarily shiftscolptrandrowvalin place, so sharing them would be a race.Testing
-t 4. Without the fix, the new tests fail and the run crashes with a segfault inklu_solve.klu!scenario above reproduces the same error directly.🤖 Generated with Claude Code