-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix inconsistency with set equality in approx_eq
#7792
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
base: main
Are you sure you want to change the base?
Conversation
pavoljuhas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix for unsortable sets and make sure the tests exercise some equal, but different-order sets; see inline comments for more details.
| types. | ||
| """ | ||
|
|
||
| if isinstance(val, set): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if isinstance(val, set): | |
| if isinstance(val, (frozenset, set)): |
And the same below.
| """ | ||
|
|
||
| if isinstance(val, set): | ||
| val = sorted(val) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will throw TypeError for set sa0 = {"a", 0}, but such set should be approx_eq to itself - approx_eq(sa0, sa0) is True.
Please add a test for unsortable sets, ie, that asserts cirq.approx_eq({"a", 0}, {"a", 0}) is true as expected.
| val = sorted(val) | |
| try: | |
| val = sorted(val) | |
| except TypeError: | |
| return NotImplemented |
and the same for other below.
You will also need to change the caller to correctly handle a NotImplemented value here -
Cirq/cirq-core/cirq/protocols/approximate_equality_protocol.py
Lines 116 to 118 in 078af27
| # If the values are iterable, try comparing recursively on items. | |
| if isinstance(val, Iterable) and isinstance(other, Iterable): | |
| return _approx_eq_iterables(val, other, atol=atol) |
| for n in range(10, 20): | ||
| assert cirq.approx_eq( | ||
| frozenset(cirq.LineQubit.range(n)), frozenset({*cirq.LineQubit.range(n)}) | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not test the new code as shown by coverage error. set and frozenset use the same hash and are constructed from the same sequence so this test works without sorting too:
for n in range(10, 20):
assert list(frozenset(cirq.LineQubit.range(n))) == list(frozenset({*cirq.LineQubit.range(n)}))To test the new code, you need to find sets for which the iteration sequence changes with insertion order:
found_differently_ordered_sets = False
for i in range(20):
for j in range(i + 1, 20):
sij = {cirq.q(i), cirq.q(j)}
sji = {cirq.q(j), cirq.q(i)}
if list(sij) != list(sji):
found_differently_ordered_sets = True
assert cirq.approx_eq(sij, sji)
assert cirq.approx_eq(frozenset(sij), sji)
assert found_differently_ordered_sets| assert cirq.approx_eq("ab", "ab", atol=1e-3) | ||
| assert not cirq.approx_eq("ab", "ac", atol=1e-3) | ||
| assert not cirq.approx_eq("1", "2", atol=999) | ||
| assert not cirq.approx_eq("test", 1, atol=1e-3) | ||
| assert not cirq.approx_eq("1", 1, atol=1e-3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert all formatting changes that convert single to double quotes. They make it harder to review the code and also make git-blame less useful.
We may decide to standardize double quotes later, but that would need its own PR (which can be added to .git-blame-ignore-revs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing, I accidentally called ruff on it. I'll revert them in my commit.
closes #6376 .