#24483 closed enhancement (fixed)
complex_field.py complex_number.pyx > complex_mpfr.pyx
Reported by:  vdelecroix  Owned by:  

Priority:  major  Milestone:  sage9.3 
Component:  basic arithmetic  Keywords:  
Cc:  ghmjungmath, chapoton  Merged in:  
Authors:  Vincent Delecroix  Reviewers:  Michael Jung 
Report Upstream:  N/A  Work issues:  
Branch:  897416c (Commits)  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
Change History (56)
comment:1 Changed 3 years ago by
 Description modified (diff)
 Report Upstream changed from Not yet reported upstream; Will do shortly. to Reported upstream. No feedback yet.
comment:2 Changed 3 years ago by
 Branch set to u/vdelecroix/24483
 Commit set to 31c7e6772e34335c149ddc3bca58540772cc3038
 Status changed from new to needs_review
comment:4 Changed 3 years ago by
 Commit changed from 31c7e6772e34335c149ddc3bca58540772cc3038 to aaab91ca3aa64742917b3e375d68c71b5adec850
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
aaab91c  24483: patch for pynac

comment:5 Changed 3 years ago by
 Commit changed from aaab91ca3aa64742917b3e375d68c71b5adec850 to f69c629b28dd1bfa0f04ac4aa0fbf3b5f1bb40ff
comment:6 Changed 3 years ago by
 Description modified (diff)
comment:7 Changed 3 years ago by
 Description modified (diff)
comment:8 Changed 3 years ago by
 Description modified (diff)
comment:9 followup: ↓ 10 Changed 3 years ago by
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 Sagespecific patches to upstream projects.
comment:10 in reply to: ↑ 9 Changed 3 years ago by
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 Sagespecific 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?
comment:11 Changed 3 years ago by
 Commit changed from f69c629b28dd1bfa0f04ac4aa0fbf3b5f1bb40ff to 74b6eab99a287cc2968e9670b8df3a7efc582d9c
comment:12 Changed 3 years ago by
All right. The import of ComplexField
from sage.rings.complex_field
is not deprecated anymore...
comment:13 Changed 3 years ago by
 Description modified (diff)
comment:14 Changed 3 years ago by
 Report Upstream changed from Reported upstream. No feedback yet. to Fixed upstream, in a later stable release.
comment:15 Changed 3 years ago by
 Dependencies set to #24497
 Report Upstream changed from Fixed upstream, in a later stable release. to N/A
comment:16 Changed 3 years ago by
 Status changed from needs_review to needs_work
Great! Thanks Ralf. I will put back the deprecation.
comment:17 Changed 3 years ago by
 Commit changed from 74b6eab99a287cc2968e9670b8df3a7efc582d9c to 14c9753e43f16686597e4a239b190472fd19ef90
comment:18 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:19 Changed 3 years ago by
 Status changed from needs_review to needs_work
a lot of doctest failures...
comment:20 Changed 3 years ago by
 Description modified (diff)
comment:21 Changed 3 years ago by
 Status changed from needs_work to needs_review
Actually these are failing because the patchbot is not merging #24497 first!
comment:22 Changed 3 years ago by
 Dependencies changed from #24497 to #24497, #22928
 Status changed from needs_review to needs_work
comment:23 Changed 3 years ago by
 Description modified (diff)
comment:24 Changed 3 years ago by
 Branch changed from u/vdelecroix/24483 to u/rws/24483
comment:25 Changed 3 years ago by
 Commit changed from 14c9753e43f16686597e4a239b190472fd19ef90 to aefb4dff3c33aa3e9378f0eb33e90070e03b44dd
 Status changed from needs_work to needs_review
comment:26 followup: ↓ 45 Changed 3 years ago by
 Status changed from needs_review to needs_work
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]
comment:27 Changed 8 weeks ago by
 Dependencies #24497, #22928 deleted
comment:28 Changed 7 weeks ago by
 Branch changed from u/rws/24483 to u/vdelecroix/24483
 Commit changed from aefb4dff3c33aa3e9378f0eb33e90070e03b44dd to c166a92144df99d08f93db548be3f89816cd6d02
 Status changed from needs_work to needs_review
comment:29 Changed 7 weeks ago by
 Milestone changed from sage8.2 to sage9.3
comment:30 Changed 7 weeks ago by
 Commit changed from c166a92144df99d08f93db548be3f89816cd6d02 to 48998db557390e068db289ad4b468d71238ac1e3
comment:31 Changed 7 weeks ago by
 Commit changed from 48998db557390e068db289ad4b468d71238ac1e3 to 2fbaa09beb6bd7fbb7a6b67a7a0439ba301ef2dd
Branch pushed to git repo; I updated commit sha1. New commits:
2fbaa09  24483: pyflakeries

comment:32 Changed 7 weeks ago by
 Commit changed from 2fbaa09beb6bd7fbb7a6b67a7a0439ba301ef2dd to 915159c47527ea77fa47f25fcd229cb3e1a62d00
Branch pushed to git repo; I updated commit sha1. New commits:
915159c  24483: Returns > Return

comment:33 Changed 7 weeks ago by
 Commit changed from 915159c47527ea77fa47f25fcd229cb3e1a62d00 to 26f2789cf97f9f9ea674e10e126367e714f59314
comment:34 Changed 7 weeks ago by
 Cc ghmjungmath added
@ghmjungmath: patchbot gave its green light! Could you do the review?
comment:35 Changed 7 weeks ago by
 Cc chapoton added
comment:36 followup: ↓ 40 Changed 7 weeks ago by
I can try. I just noticed minor things.
This one could use another line break:
return Factorization([(R(gg).monic(), e) for gg, e in zip(*F)],
f.leading_coefficient())
+
cdef class ComplexNumber(sage.structure.element.FieldElement):
"""
A floating point approximation to a complex number using any
Due to the new global import of Integer
, you can also remove the following now unnecessary imports:
sage: ComplexField().characteristic() 0 """  from .integer import Integer return Integer(0) ... 0.309016994374947 + 0.951056516295154*I """  from .integer import Integer n = Integer(n) if n == 1:
Maybe there are more of those imports, but that's what I swiftly located.
As far as I can see it, some global variables are collected in the beginning of the file while others are placed right before the corresponding class/method. I think, some sort of unification with good commenting can be beneficial for readability. But maybe that's too pedantic right now and should be part of another ticket if necessary.
comment:37 in reply to: ↑ description Changed 7 weeks ago by
Replying to vdelecroix:
Pynac used to explicitely depend on these modules but will use the more standard place
sage.rings.all
in the future (see #24497).
This comment in the ticket description perhaps needs clarification. I don't think we should be moving in the direction of encouraging imports from *.all
modules.
comment:38 Changed 7 weeks ago by
 Description modified (diff)
comment:39 Changed 7 weeks ago by
 Commit changed from 26f2789cf97f9f9ea674e10e126367e714f59314 to 03ee540227bf83b93346f78d4f9f80acdcff5d31
Branch pushed to git repo; I updated commit sha1. New commits:
03ee540  24483: unnecessary imports + clarifications

comment:40 in reply to: ↑ 36 Changed 7 weeks ago by
Replying to ghmjungmath:
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.
comment:41 followup: ↓ 43 Changed 7 weeks ago by
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.
comment:42 Changed 7 weeks ago by
 Commit changed from 03ee540227bf83b93346f78d4f9f80acdcff5d31 to 897416c8c5ca3c1e18ee4fb60f4555acbd6d18e3
Branch pushed to git repo; I updated commit sha1. New commits:
897416c  24483: import LOG_TEN_TWO_PLUS_EPSILON from sage.arith.constants

comment:43 in reply to: ↑ 41 Changed 7 weeks ago by
Replying to ghmjungmath:
The global variable
LOG_TEN_TWO_PLUS_EPSILON
can also be shifted tocreate_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 insage.rings.complex_interval
andsage.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.
comment:44 Changed 7 weeks ago by
I think, this would be it from my side. When patchbot turns green, I can give a positive review.
comment:45 in reply to: ↑ 26 Changed 7 weeks ago by
Replying to vdelecroix:
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?
comment:46 followup: ↓ 47 Changed 7 weeks ago by
This was a pretty bad doctest. It is removed in my commits

src/sage/misc/citation.pyx
diff git a/src/sage/misc/citation.pyx b/src/sage/misc/citation.pyx index 2de53c1..847f357 100644
a b systems['NTL'] = ['sage.libs.ntl', 40 40 systems['FLINT'] = ['_flint'] 41 41 systems['GMP'] = ['sage.rings.integer.Integer'] 42 42 systems['MPFR'] = ['sage.rings.real_mpfr', 43 'sage.rings.complex_ number']43 'sage.rings.complex_mpfr'] 44 44 systems['MPFI'] = ['sage.rings.real_mpfi', 45 45 'sage.rings.complex_interval'] 46 46 systems['M4RI'] = ['sage.matrix.matrix_mod2_dense'] … … def get_systems(cmd): 80 80 sage: I = R.ideal(x^2+y^2, z^2+y) 81 81 sage: get_systems('I.primary_decomposition()') 82 82 ['Singular'] 83 84 Here we get a spurious ``MPFR`` because some coercions need to be85 initialized. The second time it is gone::86 87 sage: a = var('a')88 sage: get_systems('((a+1)^2).expand()')89 ['MPFR', 'ginac']90 sage: get_systems('((a+1)^2).expand()')91 ['ginac']92 83 """ 93 84 import cProfile, pstats, re
comment:47 in reply to: ↑ 46 Changed 7 weeks ago by
Replying to vdelecroix:
This was a pretty bad doctest. It is removed in my commits
src/sage/misc/citation.pyx
diff git a/src/sage/misc/citation.pyx b/src/sage/misc/citation.pyx index 2de53c1..847f357 100644
a b systems['NTL'] = ['sage.libs.ntl', 40 40 systems['FLINT'] = ['_flint'] 41 41 systems['GMP'] = ['sage.rings.integer.Integer'] 42 42 systems['MPFR'] = ['sage.rings.real_mpfr', 43 'sage.rings.complex_ number']43 'sage.rings.complex_mpfr'] 44 44 systems['MPFI'] = ['sage.rings.real_mpfi', 45 45 'sage.rings.complex_interval'] 46 46 systems['M4RI'] = ['sage.matrix.matrix_mod2_dense'] … … def get_systems(cmd): 80 80 sage: I = R.ideal(x^2+y^2, z^2+y) 81 81 sage: get_systems('I.primary_decomposition()') 82 82 ['Singular'] 83 84 Here we get a spurious ``MPFR`` because some coercions need to be85 initialized. The second time it is gone::86 87 sage: a = var('a')88 sage: get_systems('((a+1)^2).expand()')89 ['MPFR', 'ginac']90 sage: get_systems('((a+1)^2).expand()')91 ['ginac']92 83 """ 93 84 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?
comment:48 Changed 7 weeks ago by
It might be complicated to track down precisely... with no reward as far as I can see.
comment:49 Changed 7 weeks ago by
 Status changed from needs_review to positive_review
Patchbot morally green. LGTM.
comment:50 Changed 7 weeks ago by
Thank you!
comment:51 Changed 7 weeks ago by
 Reviewers set to Michael Jung
comment:52 Changed 4 weeks ago by
 Branch changed from u/vdelecroix/24483 to 897416c8c5ca3c1e18ee4fb60f4555acbd6d18e3
 Resolution set to fixed
 Status changed from positive_review to closed
comment:53 Changed 4 weeks ago by
 Commit 897416c8c5ca3c1e18ee4fb60f4555acbd6d18e3 deleted
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?
comment:54 Changed 4 weeks ago by
Definitely (with deprecated lazy imports)!
comment:55 followup: ↓ 56 Changed 4 weeks ago by
Could you open a ticket?
comment:56 in reply to: ↑ 55 Changed 4 weeks ago by
→ #30857
Let us see about doctests
New commits:
24483: merge complex_number/complex_field into complex_mpfr
24483: fix interpreters
24483: fix imports
24483: patch for pynac