gh-36265: Raise ValueError early in Bijectionist.set_value_restrictions() when restriction is empty - #42700
Open
Chaitanya140904 wants to merge 2 commits into
Conversation
…ner product matrix The norm() method on free module elements was computing the standard Euclidean norm regardless of whether the parent had a non-trivial inner product matrix. This caused inconsistency for IntegralLattice vectors: sage: L = IntegralLattice(matrix([[1000,0],[0,1]])) sage: v = L.0 sage: v.norm() # was 1 (wrong — Euclidean norm) sage: v.inner_product(v) # was 1000 (correct) Fix: when p=2 and the parent has a non-identity inner product matrix, use inner_product(self) for the norm computation, so that norm()^2 == inner_product(self, self) consistently. Fixes: sagemath#38543
…en restriction is empty
Previously, Bijectionist.set_value_restrictions() silently stored an empty
intersection when none of the specified values appeared in Z, only raising
a ValueError later when _compute_possible_block_values() was called (i.e.
during solutions_iterator()). This made it hard to catch typos.
This commit raises a descriptive ValueError immediately in
set_value_restrictions() when the intersection of the given values with Z
is empty, giving the user an actionable message right away:
ValueError: the value restriction for element 1 is empty:
none of the given values [4] lie in Z = {1, 2, 3}
This also catches the common iterable-element mistake of passing a tuple
of allowed values instead of a list (e.g. set_value_restrictions((a, (v1,v2)))
instead of set_value_restrictions((a, [v1, v2]))).
Updated docstring with two new EXAMPLES and updated TESTS to reflect the
new (earlier) error point. The _compute_possible_block_values() test is
updated to bypass the public API directly, since the old test case no
longer reaches that method.
Fixes sagemath#36265
Contributor
|
I like your change to |
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.
Description
Bijectionist.set_value_restrictions()accepted value restrictions that produced anempty intersection with
Zwithout complaint. TheValueErrorwas only raised muchlater — deep inside
_compute_possible_block_values()whensolutions_iterator()wasalready running — making it very hard to trace back to the original typo.
What problem does this solve?
A second common pitfall: passing a tuple of values instead of a list
(iterables as elements make this especially confusing):