Opened 3 years ago

Closed 3 years ago

#22907 closed enhancement (fixed)

py3: phase out lexico cmp in real_mpfi

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.0
Component: python3 Keywords:
Cc: tscrim, jdemeyer, aapitzsch, dkrenn, cheuberg, behackl Merged in:
Authors: Frédéric Chapoton Reviewers: Daniel Krenn
Report Upstream: N/A Work issues:
Branch: ea97bfc (Commits) Commit: ea97bfce2d685fffcb7c17ac5967be875385806a
Dependencies: #18303 Stopgaps:

Description

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

Change History (16)

comment:1 Changed 3 years ago by chapoton

  • Branch set to u/chapoton/22907
  • Cc tscrim jdemeyer aapitzsch dkrenn cheuberg behackl added
  • Commit set to 20cd81e1bee338a47aa5a77906e9747df03d2592
  • Status changed from new to needs_review

comment:2 in reply to: ↑ description Changed 3 years ago by dkrenn

Replying to chapoton:

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?

comment:3 follow-up: Changed 3 years ago by chapoton

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

comment:4 in reply to: ↑ 3 ; follow-up: Changed 3 years ago by dkrenn

Replying to chapoton:

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.

comment:5 in reply to: ↑ 4 Changed 3 years ago by dkrenn

Replying to dkrenn:

Replying to chapoton:

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?

comment:6 Changed 3 years ago by dkrenn

  • Reviewers set to Daniel Krenn

comment:7 Changed 3 years ago by dkrenn

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

comment:8 Changed 3 years ago by chapoton

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

comment:9 follow-up: Changed 3 years ago by chapoton

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

comment:10 Changed 3 years ago by chapoton

  • Status changed from needs_review to needs_work

comment:11 in reply to: ↑ 9 Changed 3 years ago by dkrenn

Replying to chapoton:

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

This looks fine for me.

comment:12 Changed 3 years ago by chapoton

  • Branch changed from u/chapoton/22907 to u/chapoton/experiment-22907
  • Commit changed from 20cd81e1bee338a47aa5a77906e9747df03d2592 to ea97bfce2d685fffcb7c17ac5967be875385806a
  • Dependencies set to #18303

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

comment:13 Changed 3 years ago by chapoton

looks good, bot is morally green

comment:14 Changed 3 years ago by chapoton

  • Status changed from needs_work to needs_review

back to needs review, please double check

comment:15 Changed 3 years ago by dkrenn

  • Status changed from needs_review to positive_review

comment:16 Changed 3 years ago by vbraun

  • Branch changed from u/chapoton/experiment-22907 to ea97bfce2d685fffcb7c17ac5967be875385806a
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.