Opened 2 years ago

Last modified 18 months ago

#24457 needs_work task

modernize sage.rings.real_mpfr

Reported by: vdelecroix Owned by:
Priority: major Milestone: sage-8.4
Component: basic arithmetic Keywords:
Cc: Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by vdelecroix)

The current sage.rings.real_mpfr deals with arbitrary precision floating-point numbers (using mpfr). This is the default "real field" in many places in Sage even though it is often wiser to use intervals or balls. We perform some cosmetic changes to sage.rings.real_mpfr to emphasize the fact that the module deals with floating-point approximations of real numbers and not genuine reals. These modifications touch a lot of files and involve deprecation of widely used names.

See also the introduction of a genuine real field class in #24456 for another motivation.

step 1

  • #24511: move the class factory create_RealField in real_field.py
  • #24523: change string representation
  • #24524:
    • RealField -> RealFloatingPointField_factory
    • RealField_class -> RealFloatingPointField
    • RealNumber -> RealFloatingPoint
  • #24525: deprecate is_RealNumber(x) and is_RealField(x) in favour of isinstance(x, RealFloatingPoint) and isinstance(x, RealFloatingPointField)

step 2

  • Deprecate RR and Reals and use RFF (or RFPF?) as the standard name for RealFloatingPointField(53)?
  • more documentation.

for complex numbers, see #24489.

Change History (51)

comment:1 Changed 2 years ago by rws

  • Authors set to Ralf Stephan
  • Branch #24456 deleted
  • Dependencies set to #24456

comment:2 Changed 2 years ago by rws

In real_mpfr.pyx do you mean to rename cpdef RealField(int prec=53,...) or cdef class RealField_class?

comment:3 Changed 2 years ago by vdelecroix

  • Description modified (diff)

comment:4 Changed 2 years ago by vdelecroix

  • Description modified (diff)

comment:5 Changed 2 years ago by rws

  • Branch set to u/rws/rename_sage_rings_real_mpfr_realfield

comment:6 Changed 2 years ago by rws

  • Commit set to 71f9fd0b38f89ca57fa70f88bc80fa62d124322c
  • Status changed from new to needs_review

This commit changes 164 files. Preliminary testing passes in rings and src/doc. There might be subtle errors introduced in the documentation. Let's try the patchbots.


New commits:

62d2d2124456: class for the field of real numbers
71f9fd024457: bulk rename RealField --> RealFloatingPointField

comment:7 Changed 2 years ago by rws

Ah I forgot the string representation part.

comment:8 Changed 2 years ago by rws

  • Branch u/rws/rename_sage_rings_real_mpfr_realfield deleted
  • Commit 71f9fd0b38f89ca57fa70f88bc80fa62d124322c deleted
  • Status changed from needs_review to needs_work

Sorry, I was pretty sure I excluded real_field.py.

comment:9 Changed 2 years ago by rws

  • Branch set to u/rws/24457-1

comment:10 Changed 2 years ago by rws

  • Commit set to 8bdf66b2d1e120977cb3b0c5a04bdbfcf988a465
  • Status changed from needs_work to needs_review

After changing the string representation as well: 223 files changed, 1493 insertions(+), 1493 deletions(-). I don't have hopes this gets through, except maybe in a beta of its own.


New commits:

62d2d2124456: class for the field of real numbers
8bdf66b24457: rename sage.rings.real_mpfr.RealField

comment:11 in reply to: ↑ description ; follow-up: Changed 2 years ago by jdemeyer

Replying to vdelecroix:

  • change the name of RealField/RealField_class to RealFloatingPointField/RealFloatingPointField_class

Do we still need the old RealField given that it's no longer used after #24456?

comment:12 Changed 2 years ago by rws

  • Status changed from needs_review to needs_work

comment:13 Changed 2 years ago by rws

  • Branch changed from u/rws/24457-1 to u/rws/24457-2

comment:14 Changed 2 years ago by rws

  • Commit changed from 8bdf66b2d1e120977cb3b0c5a04bdbfcf988a465 to 3c7f16e17aa989aa2d0eeef196391a04bf1b0192
  • Status changed from needs_work to needs_review

New commits:

3c7f16e24457: rename sage.rings.real_mpfr.RealField

comment:15 in reply to: ↑ 11 ; follow-up: Changed 2 years ago by vdelecroix

Replying to jdemeyer:

Replying to vdelecroix:

  • change the name of RealField/RealField_class to RealFloatingPointField/RealFloatingPointField_class

Do we still need the old RealField given that it's no longer used after #24456?

It is. The old real field is the field of floating point numbers that is intensively used (and renamed in this ticket). The new real field is the field of real numbers (the real one).

comment:16 in reply to: ↑ 15 ; follow-ups: Changed 2 years ago by jdemeyer

Replying to vdelecroix:

The old real field is the field of floating point numbers that is intensively used (and renamed in this ticket).

I was talking about the RealField() function. Is that still used?

If so, I would rather rename it to create_RealFloatingPointField.

comment:17 in reply to: ↑ 16 ; follow-up: Changed 2 years ago by vdelecroix

Replying to jdemeyer:

Replying to vdelecroix:

The old real field is the field of floating point numbers that is intensively used (and renamed in this ticket).

I was talking about the RealField() function. Is that still used?

If so, I would rather rename it to create_RealFloatingPointField.

I think that we should get rid of this function and the hand made cache. The caching would better be handled by UniqueRepresentation by appropriately writing __classcall__/__init__. This class is very old.

comment:18 in reply to: ↑ 17 Changed 2 years ago by vdelecroix

Replying to vdelecroix:

Replying to jdemeyer:

Replying to vdelecroix:

The old real field is the field of floating point numbers that is intensively used (and renamed in this ticket).

I was talking about the RealField() function. Is that still used?

If so, I would rather rename it to create_RealFloatingPointField.

I think that we should get rid of this function and the hand made cache. The caching would better be handled by UniqueRepresentation by appropriately writing __classcall__/__init__. This class is very old.

But this is rather disjoint from this ticket.

comment:19 in reply to: ↑ 16 ; follow-up: Changed 2 years ago by vdelecroix

Replying to jdemeyer:

Replying to vdelecroix:

The old real field is the field of floating point numbers that is intensively used (and renamed in this ticket).

I was talking about the RealField() function. Is that still used?

If so, I would rather rename it to create_RealFloatingPointField.

For the sake of this ticket +1.

comment:20 in reply to: ↑ 19 ; follow-up: Changed 2 years ago by rws

Replying to vdelecroix:

Replying to jdemeyer:

If so, I would rather rename it to create_RealFloatingPointField.

For the sake of this ticket +1.

You mean "in this ticket"?

comment:21 in reply to: ↑ 20 ; follow-up: Changed 2 years ago by vdelecroix

Replying to rws:

Replying to vdelecroix:

Replying to jdemeyer:

If so, I would rather rename it to create_RealFloatingPointField.

For the sake of this ticket +1.

You mean "in this ticket"?

Sorry for my english. Yes, this is what I meant.

comment:22 in reply to: ↑ 21 Changed 2 years ago by rws

Replying to jdemeyer:

If so, I would rather rename it to create_RealFloatingPointField.

Ah I see, in rings/all.py: import create_RealFloatingPointField as RealFloatingPointField.

comment:23 Changed 2 years ago by rws

  • Branch changed from u/rws/24457-2 to u/rws/24457-3

comment:24 Changed 2 years ago by rws

  • Branch changed from u/rws/24457-3 to u/rws/24457-4

comment:25 Changed 2 years ago by git

  • Commit changed from 3c7f16e17aa989aa2d0eeef196391a04bf1b0192 to 9b2a5c2a38cf20b52dab663cecffc1009eb6ecbd

Branch pushed to git repo; I updated commit sha1. New commits:

9b2a5c224457: missing changes in interpreters

comment:26 follow-up: Changed 2 years ago by tmonteil

Some quick comments:

  • why renaming RealField and not RR at the same time ?
  • i am not sure that completely changing the behaviour of RealField without any kind of deprecation period is wise, how will the user's code survive to this change ?
  • do you really plan to change a behaviour that implies modifying 164 files without even an email to sage-devel ?
  • Regarding the naming, i would personally prefer Floating-point over Floating Point (as it is usually), moreover, it has the advantage that the shortcut will be RFF (not RFPF), consistently with other representations of real numbers.
  • where is the doc ?

comment:27 Changed 2 years ago by rws

  • Status changed from needs_review to needs_work

It's in the experimental phase. I will need an IDE with refactoring capabilities to do this reliably and quickly, or develop scripts. Also, I think it will need its own beta.

comment:28 in reply to: ↑ 26 Changed 2 years ago by vdelecroix

Replying to tmonteil:

Some quick comments:

  • why renaming RealField and not RR at the same time ?

why not. But it will

  • be a much more delicate backward incompatibility.
  • make the ticket even bigger

The 2 steps transition seems simpler to me.

  • i am not sure that completely changing the behaviour of RealField without any kind of deprecation period is wise, how will the user's code survive to this change ?

There must be deprecation about the name changes (which is not the case in the current branch). RealField shoud still point to the FloatingPointField (with deprecation) and RR should still point to RFF (with deprecation).

I don't think there is a need for deprecation about the change of printing.

  • do you really plan to change a behaviour that implies modifying 164 files without even an email to sage-devel ?
  • Regarding the naming, i would personally prefer Floating-point over Floating Point (as it is usually), moreover, it has the advantage that the shortcut will be RFF (not RFPF), consistently with other representations of real numbers.

+1 for Floating-point.

  • where is the doc ?

comment:29 Changed 2 years ago by vdelecroix

  • Dependencies #24456 deleted
  • Description modified (diff)

comment:30 Changed 2 years ago by vdelecroix

  • Description modified (diff)

comment:31 Changed 2 years ago by rws

  • Authors Ralf Stephan deleted
  • Branch u/rws/24457-4 deleted
  • Commit 9b2a5c2a38cf20b52dab663cecffc1009eb6ecbd deleted

comment:32 Changed 2 years ago by vdelecroix

  • Description modified (diff)
  • Summary changed from rename sage.rings.real_mpfr.RealField to modernize sage.rings.real_mpfr

comment:33 Changed 2 years ago by rws

I'm confused. You deleted the description item that renames RealField/RealField_class in comment:29. Does that mean those refactorings are not planned in this ticket? On the other hand you write isinstance(x, RealFloatingPointField) so the new name is used?

As to automating this and other big refactorings in Python Sage I just learned that 1. Pycharm refactoring apparently cannot be scripted (not to be expected anyway in an IDE), and 2. the rope tool is not up to the task.

comment:34 Changed 2 years ago by vdelecroix

  • Description modified (diff)

comment:35 Changed 2 years ago by rws

  • Dependencies set to #24511
  • Description modified (diff)

comment:36 follow-up: Changed 2 years ago by rws

  • Description modified (diff)

The deprecation of is_RealNumber(x)/is_RealField(x) in favor of isinstance can be done independently of the renaming. Separate ticket to come.

comment:37 in reply to: ↑ 36 ; follow-up: Changed 2 years ago by vdelecroix

Replying to rws:

The deprecation of is_RealNumber(x)/is_RealField(x) in favor of isinstance can be done independently of the renaming. Separate ticket to come.

Good idea!

comment:38 Changed 2 years ago by rws

  • Description modified (diff)

Note that is_RealField(X) then becomes isinstance(X, RealField_class).

comment:39 Changed 2 years ago by vdelecroix

  • Description modified (diff)

comment:40 Changed 2 years ago by vdelecroix

  • Description modified (diff)
  • Type changed from enhancement to task

comment:41 Changed 2 years ago by vdelecroix

Let us do that incrementally (ie each subtask in ticket). It will simplify review and merging. Though I would feel much more confortable if the whole task would be actually closed within the next stable release.

comment:42 in reply to: ↑ 37 Changed 2 years ago by vdelecroix

Replying to vdelecroix:

Replying to rws:

The deprecation of is_RealNumber(x)/is_RealField(x) in favor of isinstance can be done independently of the renaming. Separate ticket to come.

Good idea!

Though it would be better to first change the names:

  • RealField -> RealFloatingPointField_factory
  • RealField_class -> RealFloatingPointField
  • RealNumber -> RealFloatingPoint

to avoid passing twice on the same lines.

comment:43 Changed 2 years ago by vdelecroix

  • Description modified (diff)

comment:44 Changed 2 years ago by vdelecroix

  • Description modified (diff)

comment:45 Changed 2 years ago by vdelecroix

  • Description modified (diff)

comment:46 Changed 2 years ago by vdelecroix

  • Description modified (diff)

comment:47 Changed 2 years ago by vdelecroix

  • Description modified (diff)

comment:48 Changed 2 years ago by rws

  • Status changed from needs_work to needs_review

Rebased on the remaining dependency.

comment:49 Changed 2 years ago by rws

  • Status changed from needs_review to needs_work

OOps wrong ticket.

comment:50 Changed 20 months ago by vdelecroix

  • Dependencies #24511 deleted
  • Milestone changed from sage-8.2 to sage-8.3

comment:51 Changed 18 months ago by vdelecroix

  • Milestone changed from sage-8.3 to sage-8.4

update milestone 8.3 -> 8.4

Note: See TracTickets for help on using tickets.