search

sagemath / sage

Main repository of SageMath. Now open for Issues and Pull Requests.
https://www.sagemath.org
Other
1.2k stars 413 forks source link

Proper powering in CDF #27222

Closed jdemeyer closed 5 years ago

jdemeyer commented 5 years ago

Fix the Cython warning

warning: sage/rings/complex_double.pyx:1590:4: Overriding cdef method with def method.

Component: basic arithmetic

Author: Jeroen Demeyer

Branch/Commit: 2803be7

Reviewer: Travis Scrimshaw

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

jdemeyer commented 5 years ago

Description changed:

--- 
+++ 
@@ -1,5 +1,5 @@
 Fix the Cython warning

- +warning: sage/rings/complex_double.pyx:1590:4: Overriding cdef method with def method.

jdemeyer commented 5 years ago

Branch: u/jdemeyer/proper_powering_in_cdf

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Commit: 61cecb9

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

61cecb9Proper powering in CDF
tscrim commented 5 years ago
comment:5

Patchbot reports some failures:

sage -t --long src/sage/symbolic/random_tests.py  # 1 doctest failed
sage -t --long src/sage/modular/modform/numerical.py  # 6 doctests failed
jdemeyer commented 5 years ago
comment:7

There is a strange regression: CDF(-0.26319743704743886) ** -1 now returns a non-real number -3.799429094819641 - 4.652958679558198e-16*I. The old code didn't seem to do that.

jdemeyer commented 5 years ago
comment:8

GSL seems to special-case powering by a complex -1 but not by a real -1.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

7430342Proper powering in CDF
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 61cecb9 to 7430342

jdemeyer commented 5 years ago
comment:10

I added a few special cases for powering to an integer.

tscrim commented 5 years ago
comment:11

I agree with the change. Let us wait for a patchbot report.

tscrim commented 5 years ago
comment:12

Failures again reported by the patchbot:

sage -t --long src/sage/symbolic/expression.pyx  # 1 doctest failed
sage -t --long src/sage/modular/abvar/torsion_subgroup.py  # 1 doctest failed
sage -t --long src/sage/tests/french_book/mpoly.py  # 1 doctest failed

although the torsion_subgroup.py is almost certainly unrelated.

jdemeyer commented 5 years ago
comment:13

Replying to @tscrim:

the torsion_subgroup.py is almost certainly unrelated.

See #27224 for that.

The french book doctest will need to wait for #23572 (let's hope that it will finally get merged now).

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

0e0607cMinor fixes for CDF powering
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 7430342 to 0e0607c

tscrim commented 5 years ago
comment:15

So #23572 is going to be a dependency of this? I am guessing you want to wait until that is merged before proceeding.

jdemeyer commented 5 years ago

Dependencies: #23572

jdemeyer commented 5 years ago
comment:17

Replying to @tscrim:

I am guessing you want to wait until that is merged before proceeding.

Indeed. Let's hope that the next beta will have it.

jdemeyer commented 5 years ago

Changed dependencies from #23572 to none

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

d3efc0bMinor fixes for CDF powering
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 0e0607c to d3efc0b

jdemeyer commented 5 years ago
comment:20

This just fixes the doctest from french_book. If #23572 gets merged, I'll rebase it then.

tscrim commented 5 years ago

Reviewer: Travis Scrimshaw

tscrim commented 5 years ago
comment:21

Feel free to flip this to positive review if you think #23572 will take time. Although from what Volker wrote on #23572, it seems like that is easy to fix. Anyways, up to you on the order.

jdemeyer commented 5 years ago
comment:22

Replying to @tscrim:

Feel free to flip this to positive review if you think #23572 will take time.

I don't know whether it will take time. if this happens to be merged before #23572, fine. If it doesn't, also fine.

vbraun commented 5 years ago

Changed branch from u/jdemeyer/proper_powering_in_cdf to d3efc0b

vbraun commented 5 years ago

Changed branch from d3efc0b to u/jdemeyer/proper_powering_in_cdf

vbraun commented 5 years ago
comment:26

This conflicts with #23572, per your request I'm giving that one priority

jdemeyer commented 5 years ago

Dependencies: #23572

jdemeyer commented 5 years ago
comment:29

I merged #23572 and tests still pass on the patchbot.

tscrim commented 5 years ago
comment:30

I am confirming this is good.

vbraun commented 5 years ago
comment:31

Merge conflict...

jdemeyer commented 5 years ago

Changed dependencies from #23572 to none

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

9882dcaProper powering in CDF
088d0b3Minor fixes for CDF powering
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from d3efc0b to 088d0b3

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

93c8f8eProper powering in CDF
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 088d0b3 to 93c8f8e

tscrim commented 5 years ago
comment:36

Sorry, one other test I just thought of. Can you add a doctest showing the behavior of something that masquerades as an element of ZZ, such as 4/2? Once added and a green patchbot, you can set a positive review.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

2803be7Add one more example
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 93c8f8e to 2803be7

jdemeyer commented 5 years ago
comment:38

Replying to @tscrim:

Sorry, one other test I just thought of. Can you add a doctest showing the behavior of something that masquerades as an element of ZZ, such as 4/2?

I did that. As expected, this is showing a worse result than using ZZ(2) as exponent since powering by QQ uses the coercion model to actually do a powering by CDF.

tscrim commented 5 years ago
comment:39

Thank you.

vbraun commented 5 years ago

Changed branch from u/jdemeyer/proper_powering_in_cdf to 2803be7