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: phase out lexico cmp in real_mpfi #22907

Closed
fchapoton opened this issue Apr 30, 2017 · 18 comments
Closed

py3: phase out lexico cmp in real_mpfi #22907

fchapoton opened this issue Apr 30, 2017 · 18 comments

Comments

@fchapoton
Copy link
Contributor

Currently cmp(a,b) for two real-interval field elements performs a lexicographic comparison. And rich comparison has a different semantic.

We rename _cmp_ to lexico_cmp to put it outside the comparison framework. This means that cmp will not work anymore. The documentation is modified accordingly, to warn users not to use cmp at all on these objects.

Helpful for the major ticket #22297

Depends on #18303

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

Component: python3

Author: Frédéric Chapoton

Branch/Commit: ea97bfc

Reviewer: Daniel Krenn

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

@fchapoton fchapoton added this to the sage-8.0 milestone Apr 30, 2017
@fchapoton
Copy link
Contributor Author

Branch: u/chapoton/22907

@fchapoton
Copy link
Contributor Author

Commit: 20cd81e

@dkrenn
Copy link
Contributor

dkrenn commented Apr 30, 2017

Replying to @fchapoton:

Currently cmp(a,b) for two real-interval field elements performs a lexicographic comparison. And rich comparison has a different semantic.

We rename _cmp_ to lexico_cmp to put it outside the comparison framework. This means that cmp will not work anymore. The documentation is modified accordingly, to warn users not to use cmp at all on these objects.

Do we need lexico_cmp at all or could we simply delete it?

@fchapoton
Copy link
Contributor Author

comment:3

well, lexico_cmp is useful to check that pickling works. And if somebody did use cmp, it could serve as a replacement

@dkrenn
Copy link
Contributor

dkrenn commented Apr 30, 2017

comment:4

Replying to @fchapoton:

well, lexico_cmp is useful to check that pickling works. And if somebody did use cmp, it could serve as a replacement

Ok, fine for me.

@dkrenn
Copy link
Contributor

dkrenn commented Apr 30, 2017

comment:5

Replying to @dkrenn:

Replying to @fchapoton:

well, lexico_cmp is useful to check that pickling works. And if somebody did use cmp, it could serve as a replacement

As cmp is removed, do we need a deprecation warning for this?

@dkrenn
Copy link
Contributor

dkrenn commented Apr 30, 2017

Reviewer: Daniel Krenn

@dkrenn
Copy link
Contributor

dkrenn commented Apr 30, 2017

comment:7

Apart from the question above and modulo a successful run of a patchbot, this patch looks good.

@fchapoton
Copy link
Contributor Author

comment:8

Hmm, trying to introduce a deprecation seems to uncover some problems with QQbar. Investigating, maybe in relation with #18303

@fchapoton
Copy link
Contributor Author

comment:9

en experimental branch with deprecation is available as "u/chapoton/experiment-22907"

@dkrenn
Copy link
Contributor

dkrenn commented May 2, 2017

comment:11

Replying to @fchapoton:

en experimental branch with deprecation is available as "u/chapoton/experiment-22907"

This looks fine for me.

@fchapoton
Copy link
Contributor Author

Changed branch from u/chapoton/22907 to u/chapoton/experiment-22907

@fchapoton
Copy link
Contributor Author

Dependencies: #18303

@fchapoton
Copy link
Contributor Author

Changed commit from 20cd81e to ea97bfc

@fchapoton
Copy link
Contributor Author

comment:12

let us wait for #18303 and then check than nothing else is broken by deprecation


New commits:

ea97bfcpy3 deprecation of call to cmp on RIF elements

@fchapoton
Copy link
Contributor Author

comment:13

looks good, bot is morally green

@fchapoton
Copy link
Contributor Author

comment:14

back to needs review, please double check

@vbraun
Copy link
Member

vbraun commented May 16, 2017

Changed branch from u/chapoton/experiment-22907 to ea97bfc

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

3 participants