Opened 3 years ago

Closed 4 weeks ago

Last modified 4 weeks ago

#24483 closed enhancement (fixed)

complex_field.py complex_number.pyx -> complex_mpfr.pyx

Reported by: vdelecroix Owned by:
Priority: major Milestone: sage-9.3
Component: basic arithmetic Keywords:
Cc: gh-mjungmath, 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 vdelecroix)

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

Change History (56)

comment:1 Changed 3 years ago by vdelecroix

  • 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 vdelecroix

  • Branch set to u/vdelecroix/24483
  • Commit set to 31c7e6772e34335c149ddc3bca58540772cc3038
  • Status changed from new to needs_review

Let us see about doctests


New commits:

11724f724483: merge complex_number/complex_field into complex_mpfr
e314fbe24483: fix interpreters
808d0bd24483: fix imports
31c7e6724483: patch for pynac

comment:3 Changed 3 years ago by vdelecroix

  • Authors set to Vincent Delecroix

patchbot not happy if Author is not given...

comment:4 Changed 3 years ago by git

  • Commit changed from 31c7e6772e34335c149ddc3bca58540772cc3038 to aaab91ca3aa64742917b3e375d68c71b5adec850

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

aaab91c24483: patch for pynac

comment:5 Changed 3 years ago by git

  • Commit changed from aaab91ca3aa64742917b3e375d68c71b5adec850 to f69c629b28dd1bfa0f04ac4aa0fbf3b5f1bb40ff

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

a439e6f24483: fix imports and doctests
f69c62924483: patch for pynac

comment:6 Changed 3 years ago by vdelecroix

  • Description modified (diff)

comment:7 Changed 3 years ago by vdelecroix

  • Description modified (diff)

comment:8 Changed 3 years ago by vdelecroix

  • Description modified (diff)

comment:9 follow-up: Changed 3 years ago by 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.

comment:10 in reply to: ↑ 9 Changed 3 years ago by vdelecroix

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?

comment:11 Changed 3 years ago by git

  • Commit changed from f69c629b28dd1bfa0f04ac4aa0fbf3b5f1bb40ff to 74b6eab99a287cc2968e9670b8df3a7efc582d9c

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

6f2d6a224483: merge complex_number/complex_field into complex_mpfr
17092ed24483: fix interpreters
74b6eab24483: fix imports and doctests

comment:12 Changed 3 years ago by vdelecroix

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

comment:13 Changed 3 years ago by vdelecroix

  • Description modified (diff)

comment:14 Changed 3 years ago by rws

  • Report Upstream changed from Reported upstream. No feedback yet. to Fixed upstream, in a later stable release.

comment:15 Changed 3 years ago by rws

  • 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 vdelecroix

  • Status changed from needs_review to needs_work

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

comment:17 Changed 3 years ago by git

  • Commit changed from 74b6eab99a287cc2968e9670b8df3a7efc582d9c to 14c9753e43f16686597e4a239b190472fd19ef90

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

875c23124483: merge complex_number/complex_field into complex_mpfr
f3ed18c24483: fix interpreters
14c975324483: fix imports and doctests

comment:18 Changed 3 years ago by vdelecroix

  • Status changed from needs_work to needs_review

comment:19 Changed 3 years ago by vdelecroix

  • Status changed from needs_review to needs_work

a lot of doctest failures...

comment:20 Changed 3 years ago by vdelecroix

  • Description modified (diff)

comment:21 Changed 3 years ago by vdelecroix

  • 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 vdelecroix

  • Dependencies changed from #24497 to #24497, #22928
  • Status changed from needs_review to needs_work

comment:23 Changed 3 years ago by vdelecroix

  • Description modified (diff)

comment:24 Changed 3 years ago by rws

  • Branch changed from u/vdelecroix/24483 to u/rws/24483

comment:25 Changed 3 years ago by rws

  • Commit changed from 14c9753e43f16686597e4a239b190472fd19ef90 to aefb4dff3c33aa3e9378f0eb33e90070e03b44dd
  • Status changed from needs_work to needs_review

New commits:

1fdb61cMerge branch 'develop' into t/24483/24483
aefb4df24483: fix import

comment:26 follow-up: Changed 3 years ago by vdelecroix

  • 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 vdelecroix

  • Dependencies #24497, #22928 deleted

comment:28 Changed 7 weeks ago by vdelecroix

  • Branch changed from u/rws/24483 to u/vdelecroix/24483
  • Commit changed from aefb4dff3c33aa3e9378f0eb33e90070e03b44dd to c166a92144df99d08f93db548be3f89816cd6d02
  • Status changed from needs_work to needs_review

New commits:

a07cbd724483: move complex_field to complex_mpfr
2f28ddd24483: merge complex_field and complex_mpfr
c166a9224483: adapt imports and fix doctests

comment:29 Changed 7 weeks ago by vdelecroix

  • Milestone changed from sage-8.2 to sage-9.3

comment:30 Changed 7 weeks ago by git

  • Commit changed from c166a92144df99d08f93db548be3f89816cd6d02 to 48998db557390e068db289ad4b468d71238ac1e3

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

f7b400224483: fix interpreters
48998db24483: fix additional doctests

comment:31 Changed 7 weeks ago by git

  • Commit changed from 48998db557390e068db289ad4b468d71238ac1e3 to 2fbaa09beb6bd7fbb7a6b67a7a0439ba301ef2dd

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

2fbaa0924483: pyflakeries

comment:32 Changed 7 weeks ago by git

  • Commit changed from 2fbaa09beb6bd7fbb7a6b67a7a0439ba301ef2dd to 915159c47527ea77fa47f25fcd229cb3e1a62d00

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

915159c24483: Returns -> Return

comment:33 Changed 7 weeks ago by git

  • Commit changed from 915159c47527ea77fa47f25fcd229cb3e1a62d00 to 26f2789cf97f9f9ea674e10e126367e714f59314

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

e5cf2c424483: fix deprecation
26f278924483: remove complex_field from reference

comment:34 Changed 7 weeks ago by vdelecroix

  • Cc gh-mjungmath added

@gh-mjungmath: patchbot gave its green light! Could you do the review?

comment:35 Changed 7 weeks ago by vdelecroix

  • Cc chapoton added

comment:36 follow-up: Changed 7 weeks ago by gh-mjungmath

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 mkoeppe

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 vdelecroix

  • Description modified (diff)

comment:39 Changed 7 weeks ago by git

  • Commit changed from 26f2789cf97f9f9ea674e10e126367e714f59314 to 03ee540227bf83b93346f78d4f9f80acdcff5d31

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

03ee54024483: unnecessary imports + clarifications

comment:40 in reply to: ↑ 36 Changed 7 weeks ago by vdelecroix

Replying to gh-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.

comment:41 follow-up: Changed 7 weeks ago by gh-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.

Last edited 7 weeks ago by gh-mjungmath (previous) (diff)

comment:42 Changed 7 weeks ago by git

  • Commit changed from 03ee540227bf83b93346f78d4f9f80acdcff5d31 to 897416c8c5ca3c1e18ee4fb60f4555acbd6d18e3

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

897416c24483: import LOG_TEN_TWO_PLUS_EPSILON from sage.arith.constants

comment:43 in reply to: ↑ 41 Changed 7 weeks ago by vdelecroix

Replying to gh-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.

comment:44 Changed 7 weeks ago by gh-mjungmath

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 gh-mjungmath

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 follow-up: Changed 7 weeks ago by 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', 
    4040systems['FLINT'] = ['_flint']
    4141systems['GMP'] = ['sage.rings.integer.Integer']
    4242systems['MPFR'] = ['sage.rings.real_mpfr',
    43                    'sage.rings.complex_number']
     43                   'sage.rings.complex_mpfr']
    4444systems['MPFI'] = ['sage.rings.real_mpfi',
    4545                   'sage.rings.complex_interval']
    4646systems['M4RI'] = ['sage.matrix.matrix_mod2_dense']
    def get_systems(cmd): 
    8080        sage: I = R.ideal(x^2+y^2, z^2+y)
    8181        sage: get_systems('I.primary_decomposition()')
    8282        ['Singular']
    83 
    84     Here we get a spurious ``MPFR`` because some coercions need to be
    85     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']
    9283    """
    9384    import cProfile, pstats, re

comment:47 in reply to: ↑ 46 Changed 7 weeks ago by gh-mjungmath

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', 
    4040systems['FLINT'] = ['_flint']
    4141systems['GMP'] = ['sage.rings.integer.Integer']
    4242systems['MPFR'] = ['sage.rings.real_mpfr',
    43                    'sage.rings.complex_number']
     43                   'sage.rings.complex_mpfr']
    4444systems['MPFI'] = ['sage.rings.real_mpfi',
    4545                   'sage.rings.complex_interval']
    4646systems['M4RI'] = ['sage.matrix.matrix_mod2_dense']
    def get_systems(cmd): 
    8080        sage: I = R.ideal(x^2+y^2, z^2+y)
    8181        sage: get_systems('I.primary_decomposition()')
    8282        ['Singular']
    83 
    84     Here we get a spurious ``MPFR`` because some coercions need to be
    85     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']
    9283    """
    9384    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 vdelecroix

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

comment:49 Changed 7 weeks ago by gh-mjungmath

  • Status changed from needs_review to positive_review

Patchbot morally green. LGTM.

comment:50 Changed 7 weeks ago by vdelecroix

Thank you!

comment:51 Changed 7 weeks ago by gh-mjungmath

  • Reviewers set to Michael Jung

comment:52 Changed 4 weeks ago by vbraun

  • 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 mmezzarobba

  • 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 vdelecroix

Definitely (with deprecated lazy imports)!

comment:55 follow-up: Changed 4 weeks ago by vdelecroix

Could you open a ticket?

comment:56 in reply to: ↑ 55 Changed 4 weeks ago by mmezzarobba

#30857

Note: See TracTickets for help on using tickets.