Opened 5 years ago
Closed 5 years ago
#22549 closed enhancement (wontfix)
py3: change semantics of equality for real and complex interval fields
Reported by: | chapoton | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-duplicate/invalid/wontfix |
Component: | python3 | Keywords: | |
Cc: | tscrim, jdemeyer, aapitzsch, dkrenn, cheuberg, behackl | Merged in: | |
Authors: | Reviewers: | Frédéric Chapoton | |
Report Upstream: | N/A | Work issues: | |
Branch: | u/chapoton/22549 | Commit: | fd2e363687a61efd7a53279c59c2327fb8991a3e |
Dependencies: | Stopgaps: |
Description
The change is required for the transition to Python3.
Before (now), == means that both interval-numbers are exact and equal.
After, == means that both interval numbers have the same bounds.
In both cases, equality with an exact number holds only for another exact number.
Change History (29)
comment:1 Changed 5 years ago by
- Branch set to u/chapoton/22549
- Cc tscrim jdemeyer aapitzsch dkrenn cheuberg added
- Commit set to fd2e363687a61efd7a53279c59c2327fb8991a3e
comment:2 Changed 5 years ago by
- Summary changed from py3: change semantics of equality for real an complex interval fields to py3: change semantics of equality for real and complex interval fields
comment:3 in reply to: ↑ description Changed 5 years ago by
comment:4 in reply to: ↑ description Changed 5 years ago by
Replying to chapoton:
The change is required for the transition to Python3.
Before (now), == means that both interval-numbers are exact and equal.
After, == means that both interval numbers have the same bounds.
In both cases, equality with an exact number holds only for another exact number.
Why the change in the behavior of == required when switching to Python3?
comment:5 follow-up: ↓ 6 Changed 5 years ago by
For these interval-numbers, we absolutely need to get rid of the incompatible double comparison (cmp on one hand (currently lexicographic), and rich comparison (<,>,==,!=,<=,=>, currently with the semantics "all elements are related to all elements") on the other hand).
Right now "cmp" is used very deeply to make sure that objects have UniqueRepresentation? or can be pickled correctly. In order for all this to work, we need to relax the richcmp behaviour of equality to match the current behaviour of equality with cmp.
An illustrating problem is the following : take an interval say (0.1,0.2). Pickle it. Load it again. According to the current rich comparison, it will not be equal to itself. This breaks many things.
If one of you think he can get quickly an alternative easy solution, I engage him or her to try. I can tell you that I did. IMHO, the current proposal is the least invasive solution.
comment:6 in reply to: ↑ 5 Changed 5 years ago by
Replying to chapoton:
For these interval-numbers, we absolutely need to get rid of the incompatible double comparison (cmp on one hand (currently lexicographic), and rich comparison (<,>,==,!=,<=,=>, currently with the semantics "all elements are related to all elements") on the other hand).
I agree. However, I would rather keep the current rich comparison and drop the old-style cmp
.
Right now "cmp" is used very deeply to make sure that objects have UniqueRepresentation?
UniqueRepresentation
deals with parents. Here we are talking about elements, so I don't see the issue.
or can be pickled correctly.
How does pickling involve cmp()
?
An illustrating problem is the following : take an interval say (0.1,0.2). Pickle it. Load it again. According to the current rich comparison, it will not be equal to itself.
Feature, not a bug.
This breaks many things.
Like what?
comment:7 follow-up: ↓ 8 Changed 5 years ago by
Just look at the patchbot reports of #22257
comment:8 in reply to: ↑ 7 Changed 5 years ago by
comment:9 follow-ups: ↓ 11 ↓ 12 Changed 5 years ago by
#22257 tells you what happens if you replace current cmp behaviour by current richcmp behaviour. If you think you can repair all the breaking doctests there by doing something else, please try. I have not found any other solution.
The solution proposed here almost passes all tests. The few failing doctests are mostly expected due to the change of behaviour, and do no harm. There is one more complex failure related to composite of number fields. I would like to have the opinion of a number theorist on this one.
comment:10 follow-up: ↓ 14 Changed 5 years ago by
This is really involved in unique representation and ComparisonById?, because Number Fields (which are Parent) have embeddings (which are interval-numbers) that need to compare equal to themselves for the number fields to do the same.
For some triggered failures by using the current richcmp, see precisely this report:
comment:11 in reply to: ↑ 9 ; follow-up: ↓ 13 Changed 5 years ago by
Replying to chapoton:
The solution proposed here almost passes all tests. The few failing doctests are mostly expected due to the change of behaviour, and do no harm. There is one more complex failure related to composite of number fields. I would like to have the opinion of a number theorist on this one.
Tons of external code might be broken by this change of behaviour. Changing the semantics of ==
at this point is completely inacceptable for me.
comment:12 in reply to: ↑ 9 ; follow-ups: ↓ 19 ↓ 28 Changed 5 years ago by
Replying to chapoton:
#22257 tells you what happens if you replace current cmp behaviour by current richcmp behaviour.
Here is a constructive suggestion: essentially, what cmp()
does on intervals is comparing the endpoints. So whenever you need to replace cmp()
for intervals by rich comparison, use rich comparison on the endpoints. For example, you can replace
cmp(x, y) < 0
by
x.endpoints() < y.endpoints()
For x.endpoints() == y.endpoints()
, it makes sense to define a new method equals()
on intervals which does this.
comment:13 in reply to: ↑ 11 ; follow-up: ↓ 21 Changed 5 years ago by
Replying to cheuberg:
Tons of external code might be broken by this change of behaviour. Changing the semantics of
==
at this point is completely inacceptable for me.
+1
comment:14 in reply to: ↑ 10 Changed 5 years ago by
Replying to chapoton:
This is really involved in unique representation and ComparisonById?, because Number Fields (which are Parent) have embeddings (which are interval-numbers) that need to compare equal to themselves for the number fields to do the same.
Could we replace checking for equality to checking for non-inequality instead?
comment:15 follow-up: ↓ 17 Changed 5 years ago by
Yes, guys, I understand your issues. Nevertheless, I have worked hard on the question, and this really stands in our way to python3. I do not think that this proposal is such a big change of behaviour. Maybe most equality tests between elements of RIF should involve an exact element, no ?
Transition to python3 is going to be painful, but you really appreciate that when spending a lot of time on this transition, as I did.
comment:16 follow-up: ↓ 18 Changed 5 years ago by
If I read the code correctly, the proposed behaviour is the same as the current behaviour for real and complex ball fields, so it should not be so bad.
comment:17 in reply to: ↑ 15 Changed 5 years ago by
Replying to chapoton:
Nevertheless, I have worked hard on the question, and this really stands in our way to python3.
So far, I totally agree.
I do not think that this proposal is such a big change of behaviour.
I think it's a massive change of behaviour which really should not be done unless you have very good reasons (so far, I haven't seen such a reason).
Maybe most equality tests between elements of RIF should involve an exact element, no ?
Probably yes.
Transition to python3 is going to be painful, but you really appreciate that when spending a lot of time on this transition, as I did.
Again, I totally agree. Still, the fact that it's painful does not mean that we should go ahead with what you propose here.
comment:18 in reply to: ↑ 16 ; follow-up: ↓ 20 Changed 5 years ago by
Replying to chapoton:
the proposed behaviour is the same as the current behaviour for real and complex ball fields
This is simply not true:
sage: x = RIF.pi() sage: x == x False sage: RBF(x) == RBF(x) False
comment:19 in reply to: ↑ 12 Changed 5 years ago by
Replying to jdemeyer:
For
x.endpoints() == y.endpoints()
, it makes sense to define a new methodequals()
on intervals which does this.
I'd suggest calling it identical()
, for consistency with real and complex balls.
comment:20 in reply to: ↑ 18 Changed 5 years ago by
Indeed, I was wrong on that point. The doc of ball fields says
Two elements are equal if and only if they are the same object or if both are exact and equal::
so that
sage: x=RBF(pi) sage: x==x True
Replying to jdemeyer:
Replying to chapoton:
the proposed behaviour is the same as the current behaviour for real and complex ball fields
This is simply not true:
sage: x = RIF.pi() sage: x == x False sage: RBF(x) == RBF(x) False
comment:21 in reply to: ↑ 13 Changed 5 years ago by
comment:22 Changed 5 years ago by
- Cc behackl added
comment:23 Changed 5 years ago by
Ok. It seems that the solution proposed here will not be accepted. I will now try to go back to #22257 and propose there another fix with no change to the == semantics.
comment:24 Changed 5 years ago by
I would say what needs to be fixed is what is stored as the caching key for number fields. In this case, it looks like the construction of said key, when given an element of RIF/RBF, should instead store the endpoints. The parent is reconstructible from that data and it doesn't involve the x == x
being False
issue.
comment:25 Changed 5 years ago by
- Milestone changed from sage-7.6 to sage-duplicate/invalid/wontfix
- Reviewers set to Frédéric Chapoton
- Status changed from new to needs_review
comment:26 Changed 5 years ago by
- Status changed from needs_review to positive_review
comment:27 Changed 5 years ago by
yes, I agree, you can close as wontfix
comment:28 in reply to: ↑ 12 Changed 5 years ago by
Replying to jdemeyer:
Here is a constructive suggestion: essentially, what
cmp()
does on intervals is comparing the endpoints.
Note however that we have:
sage: cmp(RIF(1/3), RIF('nan')) 0
IMO this is a bug or at least a misfeature of the current _cmp_()
, but who knows what might rely on it... (I had code that did by accident.)
comment:29 Changed 5 years ago by
- Resolution set to wontfix
- Status changed from positive_review to closed
Closing tickets in the sage-duplicate/invalid/wontfix module with positive_review (i.e. someone has confirmed they should be closed).
Work in progress. In particular doc still needs to be adapted.
New commits:
change slighty semantics of equality in real mpfi
cmp in real mpfi : doctests
py3 : removal of cmp() in real_lazy
Merge branch 'u/chapoton/22257' in real_mpfi branch
py3: change semantics of equality for real and cplx interval fields