Correct number of srcs in complete
#2468
Merged
Conversation
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
When constructing the inverse match, `complete` was allocating a vector with length of the forward match. This isn't quite the right number. What you want is to use the actual number of srcs (or failing that the maximum number that appears in the matching). In practice, this was mostly working out, because an overestimate is fine here and usually there are more variables than equations, but there don't have to be in all places where Matching is used. Fix that by defaulting `complete` to the maximum used src and explicitly passing the correct number of sources in a few places.
Keno
force-pushed
the
kf/completedefault
branch
from
a7bd421 to
23c010b
Compare
YingboMa
reviewed
Feb 15, 2024
| @@ -88,7 +88,7 @@ function Base.push!(m::Matching, v) | |||
| end | |||
| end | |||
|
|
|||
| function complete(m::Matching{U}, N = length(m.match)) where {U} | |||
| function complete(m::Matching{U}, N = maximum((x for x in m.match if isa(x, Int)); init=0)) where {U} | |||
There was a problem hiding this comment.
Handles the case when m is empty.
|
@YingboMa good to go? |
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.
When constructing the inverse match,
completewas allocating a vector with length of the forward match. This isn't quite the right number. What you want is to use the actual number of srcs (or failing that the maximum number that appears in the matching). In practice, this was mostly working out, because an overestimate is fine here and usually there are more variables than equations, but there don't have to be in all places where Matching is used. Fix that by defaultingcompleteto the maximum used src and explicitly passing the correct number of sources in a few places.