search

sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.45k stars 482 forks source link

py3: phase out lexico cmp in real_mpfi #22907

Closed fchapoton closed 7 years ago

fchapoton commented 7 years ago

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 commented 7 years ago

Branch: u/chapoton/22907

fchapoton commented 7 years ago

Commit: 20cd81e

dkrenn commented 7 years ago

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 commented 7 years ago
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 commented 7 years ago
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 commented 7 years ago
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 commented 7 years ago

Reviewer: Daniel Krenn

dkrenn commented 7 years ago
comment:7

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

fchapoton commented 7 years ago
comment:8

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

fchapoton commented 7 years ago
comment:9

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

dkrenn commented 7 years ago
comment:11

Replying to @fchapoton:

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

This looks fine for me.

fchapoton commented 7 years ago

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

fchapoton commented 7 years ago

Dependencies: #18303

fchapoton commented 7 years ago

Changed commit from 20cd81e to ea97bfc

fchapoton commented 7 years ago
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 commented 7 years ago
comment:13

looks good, bot is morally green

fchapoton commented 7 years ago
comment:14

back to needs review, please double check

vbraun commented 7 years ago

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