Skip to content
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

py3: change semantics of equality for real and complex interval fields #22549

Closed
fchapoton opened this issue Mar 8, 2017 · 29 comments
Closed

Comments

@fchapoton
Copy link
Contributor

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.

CC: @tscrim @jdemeyer @a-andre @dkrenn @cheuberg @behackl

Component: python3

Branch/Commit: u/chapoton/22549 @ fd2e363

Reviewer: Frédéric Chapoton

Issue created by migration from https://trac.sagemath.org/ticket/22549

@fchapoton fchapoton added this to the sage-7.6 milestone Mar 8, 2017
@fchapoton
Copy link
Contributor Author

Branch: u/chapoton/22549

@fchapoton
Copy link
Contributor Author

comment:1

Work in progress. In particular doc still needs to be adapted.


New commits:

c156606change slighty semantics of equality in real mpfi
6d2bf26cmp in real mpfi : doctests
6a919fapy3 : removal of cmp() in real_lazy
7f5c49aMerge branch 'u/chapoton/22257' in real_mpfi branch
fd2e363py3: change semantics of equality for real and cplx interval fields

@fchapoton
Copy link
Contributor Author

Commit: fd2e363

@fchapoton fchapoton changed the title py3: change semantics of equality for real an complex interval fields py3: change semantics of equality for real and complex interval fields Mar 8, 2017
@jdemeyer
Copy link

jdemeyer commented Mar 8, 2017

Replying to @fchapoton:

The change is required for the transition to Python3.

Please elaborate.

@dkrenn
Copy link
Contributor

dkrenn commented Mar 8, 2017

Replying to @fchapoton:

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?

@fchapoton
Copy link
Contributor Author

comment:5

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.

@jdemeyer
Copy link

jdemeyer commented Mar 8, 2017

comment:6

Replying to @fchapoton:

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?

@fchapoton
Copy link
Contributor Author

comment:7

Just look at the patchbot reports of #22257

@jdemeyer
Copy link

jdemeyer commented Mar 8, 2017

comment:8

Replying to @fchapoton:

Just look at the patchbot reports of #22257

That just shows that #22257 should be fixed. I don't see why it would require to change the semantics of == on intervals.

@fchapoton
Copy link
Contributor Author

comment:9

#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.

@fchapoton
Copy link
Contributor Author

comment:10

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:

https://patchbot.sagemath.org/log/22257/Ubuntu/16.04/x86_64/4.4.0-53-generic/petitbonum/2017-01-29%2003:55:48?short

@cheuberg
Copy link
Contributor

cheuberg commented Mar 8, 2017

comment:11

Replying to @fchapoton:

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.

@jdemeyer
Copy link

jdemeyer commented Mar 8, 2017

comment:12

Replying to @fchapoton:

#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.

@jdemeyer
Copy link

jdemeyer commented Mar 8, 2017

comment:13

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

@jdemeyer
Copy link

jdemeyer commented Mar 8, 2017

comment:14

Replying to @fchapoton:

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?

@fchapoton
Copy link
Contributor Author

comment:15

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.

@fchapoton
Copy link
Contributor Author

comment:16

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.

@jdemeyer
Copy link

jdemeyer commented Mar 8, 2017

comment:17

Replying to @fchapoton:

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.

@jdemeyer
Copy link

jdemeyer commented Mar 8, 2017

comment:18

Replying to @fchapoton:

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

@mezzarobba
Copy link
Member

comment:19

Replying to @jdemeyer:

For x.endpoints() == y.endpoints(), it makes sense to define a new method equals() on intervals which does this.

I'd suggest calling it identical(), for consistency with real and complex balls.

@fchapoton
Copy link
Contributor Author

comment:20

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 @fchapoton:

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

@dkrenn
Copy link
Contributor

dkrenn commented Mar 8, 2017

comment:21

Replying to @jdemeyer:

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

+1 as well

@fchapoton
Copy link
Contributor Author

comment:23

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.

@tscrim
Copy link
Collaborator

tscrim commented Mar 8, 2017

comment:24

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.

@jdemeyer
Copy link

jdemeyer commented Mar 9, 2017

Changed author from Frédéric Chapoton to none

@jdemeyer
Copy link

jdemeyer commented Mar 9, 2017

Reviewer: Frédéric Chapoton

@fchapoton
Copy link
Contributor Author

comment:27

yes, I agree, you can close as wontfix

@mezzarobba
Copy link
Member

comment:28

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.)

@embray
Copy link
Contributor

embray commented Jul 13, 2017

comment:29

Closing tickets in the sage-duplicate/invalid/wontfix module with positive_review (i.e. someone has confirmed they should be closed).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants