complex_field.py complex_number.pyx -> complex_mpfr.pyx #24483

videlec · 2018-01-07T13:25:10Z

In order to uniformize, simplify and in view of #17713 we merge the two files complex_field.py, complex_number.pyx into a unique complex_mpfr.pyx.

follow-up: #24489

CC: @mjungmath @fchapoton

Component: basic arithmetic

Author: Vincent Delecroix

Branch: 897416c

Reviewer: Michael Jung

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

The text was updated successfully, but these errors were encountered:

videlec · 2018-01-07T13:41:07Z

Changed upstream from Not yet reported upstream; Will do shortly. to Reported upstream. No feedback yet.

videlec · 2018-01-07T15:43:07Z

Commit: 31c7e67

videlec · 2018-01-07T15:43:07Z

comment:2

Let us see about doctests

New commits:

`11724f7`	`24483: merge complex_number/complex_field into complex_mpfr`
`e314fbe`	`24483: fix interpreters`
`808d0bd`	`24483: fix imports`
`31c7e67`	`24483: patch for pynac`

videlec · 2018-01-07T15:43:07Z

Branch: u/vdelecroix/24483

videlec · 2018-01-07T17:31:20Z

Author: Vincent Delecroix

videlec · 2018-01-07T17:31:20Z

comment:3

patchbot not happy if Author is not given...

sagetrac-git · 2018-01-07T18:59:20Z

Changed commit from 31c7e67 to aaab91c

sagetrac-git · 2018-01-07T18:59:20Z

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

`aaab91c`	`24483: patch for pynac`

sagetrac-git · 2018-01-08T08:40:06Z

Changed commit from aaab91c to f69c629

sagetrac-git · 2018-01-08T08:40:06Z

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

`a439e6f`	`24483: fix imports and doctests`
`f69c629`	`24483: patch for pynac`

jdemeyer · 2018-01-08T11:00:38Z

comment:9

Can we just do the renaming without deprecation for now? That way, we wouldn't need the fix the Pynac. If possible, I would like to avoid Sage-specific patches to upstream projects.

videlec · 2018-01-08T11:03:44Z

comment:10

Replying to @jdemeyer:

Can we just do the renaming without deprecation for now? That way, we wouldn't need the fix the Pynac. If possible, I would like to avoid Sage-specific patches to upstream projects.

The whole point is precisely to start a deprecation... though for ease of review I can move the deprecation in another ticket. Would that be better?

sagetrac-git · 2018-01-08T14:56:15Z

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

`6f2d6a2`	`24483: merge complex_number/complex_field into complex_mpfr`
`17092ed`	`24483: fix interpreters`
`74b6eab`	`24483: fix imports and doctests`

sagetrac-git · 2018-01-08T14:56:15Z

Changed commit from f69c629 to 74b6eab

videlec · 2018-01-08T14:56:42Z

comment:12

All right. The import of ComplexField from sage.rings.complex_field is not deprecated anymore...

rwst · 2018-01-08T16:17:03Z

Changed upstream from Reported upstream. No feedback yet. to Fixed upstream, in a later stable release.

rwst · 2018-01-09T09:21:22Z

Changed upstream from Fixed upstream, in a later stable release. to none

rwst · 2018-01-09T09:21:22Z

Dependencies: #24497

videlec · 2018-01-09T09:46:32Z

comment:16

Great! Thanks Ralf. I will put back the deprecation.

sagetrac-git · 2020-10-11T07:24:31Z

Changed commit from 26f2789 to 03ee540

videlec · 2020-10-11T07:26:02Z

comment:40

Replying to @mjungmath:

I can try. I just noticed minor things.

[...]

Thank you. I moved around the late_import. The remaining cache global variable is used in the function just after. It makes sense to keep it there as it is not used anywhere else in the code.

mjungmath · 2020-10-11T09:23:48Z

comment:41

The global variable LOG_TEN_TWO_PLUS_EPSILON can also be shifted to create_ComplexNumber since this is the only place where it is used.

Besides, this constant is already defined in sage.arith.constants, but with a slightly different value it seems. However, this value is imported in sage.rings.complex_interval and sage.rings.real_mpfi, i.e. everywhere else, but for the very same purpose. If it doesn't affect the code too much, one can import this value from there as well for the sake of unification.

sagetrac-git · 2020-10-11T09:35:59Z

Changed commit from 03ee540 to 897416c

sagetrac-git · 2020-10-11T09:35:59Z

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

`897416c`	`24483: import LOG_TEN_TWO_PLUS_EPSILON from sage.arith.constants`

videlec · 2020-10-11T09:36:25Z

comment:43

Replying to @mjungmath:

The global variable LOG_TEN_TWO_PLUS_EPSILON can also be shifted to create_ComplexNumber since this is the only place where it is used.

Besides, this constant is already defined in sage.arith.constants, but with a slightly different value it seems. However, this value is imported in sage.rings.complex_interval and sage.rings.real_mpfi, i.e. everywhere else, but for the very same purpose. If it doesn't affect the code too much, one can import this value from there as well for the sake of unification.

True. Fixed in the last commit.

mjungmath · 2020-10-11T09:43:19Z

comment:44

I think, this would be it from my side. When patchbot turns green, I can give a positive review.

mjungmath · 2020-10-11T10:54:55Z

comment:45

Replying to @videlec:

Got one doctest failure

sage -t src/sage/misc/citation.pyx
**********************************************************************
File "src/sage/misc/citation.pyx", line 87, in sage.misc.citation.get_systems
Failed example:
    get_systems('((a+1)^2).expand()')
Expected:
    ['MPFR', 'ginac']
Got:
    ['ginac']
**********************************************************************
1 item had failures:
   1 of  11 in sage.misc.citation.get_systems
    [13 tests, 1 failure, 1.75 s]

Is it worth to investigate why this (former) "spurious" output is gone now?

videlec · 2020-10-11T10:58:28Z

comment:46

This was a pretty bad doctest. It is removed in my commits

diff --git a/src/sage/misc/citation.pyx b/src/sage/misc/citation.pyx
index 2de53c1..847f357 100644
--- a/src/sage/misc/citation.pyx
+++ b/src/sage/misc/citation.pyx
@@ -40,7 +40,7 @@ systems['NTL'] = ['sage.libs.ntl',
 systems['FLINT'] = ['_flint']
 systems['GMP'] = ['sage.rings.integer.Integer']
 systems['MPFR'] = ['sage.rings.real_mpfr',
-                   'sage.rings.complex_number']
+                   'sage.rings.complex_mpfr']
 systems['MPFI'] = ['sage.rings.real_mpfi',
                    'sage.rings.complex_interval']
 systems['M4RI'] = ['sage.matrix.matrix_mod2_dense']
@@ -80,15 +80,6 @@ def get_systems(cmd):
         sage: I = R.ideal(x^2+y^2, z^2+y)
         sage: get_systems('I.primary_decomposition()')
         ['Singular']
-
-    Here we get a spurious ``MPFR`` because some coercions need to be
-    initialized. The second time it is gone::
-
-        sage: a = var('a')
-        sage: get_systems('((a+1)^2).expand()')
-        ['MPFR', 'ginac']
-        sage: get_systems('((a+1)^2).expand()')
-        ['ginac']
     """
     import cProfile, pstats, re

mjungmath · 2020-10-11T11:24:40Z

comment:47

Replying to @videlec:

This was a pretty bad doctest. It is removed in my commits

diff --git a/src/sage/misc/citation.pyx b/src/sage/misc/citation.pyx
index 2de53c1..847f357 100644
--- a/src/sage/misc/citation.pyx
+++ b/src/sage/misc/citation.pyx
@@ -40,7 +40,7 @@ systems['NTL'] = ['sage.libs.ntl',
 systems['FLINT'] = ['_flint']
 systems['GMP'] = ['sage.rings.integer.Integer']
 systems['MPFR'] = ['sage.rings.real_mpfr',
-                   'sage.rings.complex_number']
+                   'sage.rings.complex_mpfr']
 systems['MPFI'] = ['sage.rings.real_mpfi',
                    'sage.rings.complex_interval']
 systems['M4RI'] = ['sage.matrix.matrix_mod2_dense']
@@ -80,15 +80,6 @@ def get_systems(cmd):
         sage: I = R.ideal(x^2+y^2, z^2+y)
         sage: get_systems('I.primary_decomposition()')
         ['Singular']
-
-    Here we get a spurious ``MPFR`` because some coercions need to be
-    initialized. The second time it is gone::
-
-        sage: a = var('a')
-        sage: get_systems('((a+1)^2).expand()')
-        ['MPFR', 'ginac']
-        sage: get_systems('((a+1)^2).expand()')
-        ['ginac']
     """
     import cProfile, pstats, re

Thanks. Removing the test seems sensible. However, I just ask myself why this ticket has that effect. Perhaps it is worth to investigate it?

videlec · 2020-10-11T11:28:41Z

comment:48

It might be complicated to track down precisely... with no reward as far as I can see.

mjungmath · 2020-10-11T12:08:51Z

comment:49

Patchbot morally green. LGTM.

videlec · 2020-10-11T12:11:11Z

comment:50

Thank you!

mjungmath · 2020-10-11T18:32:14Z

Reviewer: Michael Jung

vbraun · 2020-10-31T18:07:49Z

Changed branch from u/vdelecroix/24483 to 897416c

mezzarobba · 2020-11-02T17:35:11Z

comment:53

Hi Vincent,

What would you think of keeping files complex_field.py and complex_number.pyx that import * from the new location for a while, in the interest of backward compatibility?

mezzarobba · 2020-11-02T17:35:11Z

Changed commit from 897416c to none

videlec · 2020-11-02T17:49:29Z

comment:54

Definitely (with deprecated lazy imports)!

videlec · 2020-11-02T17:50:39Z

comment:55

Could you open a ticket?

mezzarobba · 2020-11-03T16:25:13Z

comment:56

→ #30857

…(2020)

… (2020)

, sagemath#24483, sagemath#24371, sagemath#24511, sagemath#25848, sagemath#26105, sagemath#28481, sagemath#29010, sagemath#29412, sagemath#30332, sagemath#30372, sagemath#31345, sagemath#32375, sagemath#32606, sagemath#32610, sagemath#32612, sagemath#32641, sagemath#32660, sagemath#32750, sagemath#32869, sagemath#33602      ### 📝 Checklist     - [x] The title is concise, informative, and self-explanatory. - [ ] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies   URL: sagemath#36307 Reported by: Matthias Köppe Reviewer(s):

videlec added this to the sage-8.2 milestone Jan 7, 2018

videlec added c: basic arithmetic labels Jan 7, 2018

This comment has been minimized.

Sign in to view

videlec added the s: needs review label Jan 7, 2018

This comment has been minimized.

Sign in to view

videlec removed the s: needs review label Jan 9, 2018

mjungmath added s: positive review and removed s: needs review labels Oct 11, 2020

vbraun removed the s: positive review label Oct 31, 2020

vbraun closed this as completed in f4f4fcb Oct 31, 2020

rwst mentioned this issue Jan 7, 2021

Meta-ticket: Towards a genuine RealField #17713

Open

This was referenced Dec 29, 2022

classes for the fields of complex and real numbers #24456

Open

modernize complex_mpfr #24489

Open

mkoeppe added a commit to mkoeppe/sage that referenced this issue Sep 21, 2023

src/sage/rings/complex_field.py: Remove; deprecated in sagemath#24483 …

ad80dfa

…(2020)

mkoeppe added a commit to mkoeppe/sage that referenced this issue Sep 21, 2023

src/sage/rings/complex_number.pyx: Remove; deprecated in sagemath#24483…

c91d21e

… (2020)

complex_field.py complex_number.pyx -> complex_mpfr.pyx #24483

complex_field.py complex_number.pyx -> complex_mpfr.pyx #24483

Comments

videlec commented Jan 7, 2018

videlec commented Jan 7, 2018

This comment has been minimized.

videlec commented Jan 7, 2018

videlec commented Jan 7, 2018

videlec commented Jan 7, 2018

videlec commented Jan 7, 2018

videlec commented Jan 7, 2018

sagetrac-git mannequin commented Jan 7, 2018

sagetrac-git mannequin commented Jan 7, 2018

sagetrac-git mannequin commented Jan 8, 2018

sagetrac-git mannequin commented Jan 8, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdemeyer commented Jan 8, 2018

videlec commented Jan 8, 2018

sagetrac-git mannequin commented Jan 8, 2018

sagetrac-git mannequin commented Jan 8, 2018

videlec commented Jan 8, 2018

This comment has been minimized.

rwst commented Jan 8, 2018

rwst commented Jan 9, 2018

rwst commented Jan 9, 2018

videlec commented Jan 9, 2018

sagetrac-git mannequin commented Oct 11, 2020

videlec commented Oct 11, 2020

mjungmath commented Oct 11, 2020

sagetrac-git mannequin commented Oct 11, 2020

sagetrac-git mannequin commented Oct 11, 2020

videlec commented Oct 11, 2020

mjungmath commented Oct 11, 2020

mjungmath commented Oct 11, 2020

videlec commented Oct 11, 2020

mjungmath commented Oct 11, 2020

videlec commented Oct 11, 2020

mjungmath commented Oct 11, 2020

videlec commented Oct 11, 2020

mjungmath commented Oct 11, 2020

vbraun commented Oct 31, 2020

mezzarobba commented Nov 2, 2020

mezzarobba commented Nov 2, 2020

videlec commented Nov 2, 2020

videlec commented Nov 2, 2020

mezzarobba commented Nov 3, 2020