Give deprecation warning when raising negative AlgebraicReal to fractional odd power #38362
Conversation
|
Documentation preview for this PR (built with commit a5a2fbe; changes) is ready! 🎉 |
|
Would you consider #38564? |
I believe we should change the semantics of fractional powers in |
|
This PR and my PR #38564 both propose changes in the semantics of AA(symbolic expression). We cannot view the current behavior as a bug. That is just a different interpretation of fractional powers of negative reals, which number theorists find inconvenient Hence I think we should follow the deprecation rules. For a year, we advertise the upcoming semantics change. Then after a year, we switch to the semantics we want. To implement that, I suggest to use this PR for the deprecation purpose, and after the deprecation period use my PR to change the semantics. What do you think? (well, the deprecation period is painful, but that is the rule...) |
|
By the way, I think one-year deprecation period is too long, in general. A half year would be enough. |
|
On Tuesday, 27 August 2024 05:12:49 CEST Kwankyu Lee wrote:
We cannot view the current behavior as a bug.
Not in isolation, but I'd argue that the current semantics is wrong
because it breaks the assumptions of the coercion system, and, more
generally, the overall consistency of the Sage library (one expects
methods with the same name applied to mathematically equivalent objects
to behave in a consistent way).
(I'm not trying to argue for bypassing the deprecation rules here, just
trying to explain why I believe it would be acceptable.)
To implement that, I suggest to use this PR for the deprecation
purpose, and after the deprecation period use my PR to change the
semantics. What do you think?
Sounds good to me.
…--
Marc
|
I agree with that, why we are working here. |
|
Hmm. It seems that the cause of the "bug" in #36735 is elsewhere... See this: sage: RR(-1)^(1/3)
0.500000000000000 + 0.866025403784439*I
sage: CC(-1)^(1/3)
0.500000000000000 + 0.866025403784439*I
sage: QQbar(-1)^(1/3)
0.500000000000000? + 0.866025403784439?*I
sage: AA(-1)^(1/3) # first inconsistency
-1and also sage: QQ(-1)^(1/3)
(-1)^(1/3)
sage: ZZ(-1)^(1/3)
(-1)^(1/3)Fixing this inconsistency alone might solve #36735. On the other hand, there is another kind of inconsistency: sage: RR((-1)^(1/3))
...
TypeError: unable to convert '0.500000000000000+0.866025403784439*I' to a real number
sage: AA((-1)^(1/3)) # second inconsistency
-1This PR and my PR #38564 both solve the second inconsistency. #36735 is also solved kind-of as a side effect. |
|
In accordance with the opinion in https://groups.google.com/g/sage-devel/c/s81ieq2vpVo, I also fixed the first inconsistency in #38564. @user202729 Are you willing to repurpose this PR for appropriate deprecation warnings? |
|
Alright, I try to change the code accordingly. If you also want diff --git a/src/sage/rings/qqbar.py b/src/sage/rings/qqbar.py
index 5c664d53e71..9d66e65a910 100644
--- a/src/sage/rings/qqbar.py
+++ b/src/sage/rings/qqbar.py
@@ -6430,6 +6430,9 @@ class AlgebraicNumberPowQQAction(Action):
S = self.codomain()
if S is AA and d % 2 == 0 and x.sign() < 0:
S = QQbar
+ elif S is AA and d % 2 != 0 and d != 1 and x.sign() < 0:
+ from sage.misc.superseded import deprecation
+ deprecation(36735, "Raising negative algebraic real number to odd fractional power is deprecated")
# First, check for exact roots.
if isinstance(x._descr, ANRational):
diff --git a/src/sage/symbolic/expression_conversion_algebraic.py b/src/sage/symbolic/expression_co>
index fd9c9066c9e..b2d842ddbc3 100644
--- a/src/sage/symbolic/expression_conversion_algebraic.py
+++ b/src/sage/symbolic/expression_conversion_algebraic.py
@@ -130,10 +130,6 @@ class AlgebraicConverter(Converter):
base, expt = ex.operands()
base = self.field(base)
expt = Rational(expt)
- if (isinstance(self.field, AlgebraicRealField) and expt.denom() != 1 and
- expt.denom() % 2 != 0 and base < 0):
- from sage.misc.superseded import deprecation
- deprecation(36735, "Conversion of negative number to fractional power to AA is>
return self.field(base**expt)
else:
if operator is add_vararg:While looking at it there's also this behavior… sage: AA(-sqrt(27)+1)^(1/2)
2.048451225366773?*I
sage: _.base_ring()
Algebraic Real Field
sage: AA(-1)^(1/2)
I
sage: _.base_ring()
Algebraic Real FieldSurely the element should belong to its base ring? Though the problem is should it give an error or silently convert to |
No, it does not: It looks like it's a shortcut for I'm not sure I agree (I think of QQbar as not particularly related to any complex embedding), but given that the implementation really does realize QQbar as a subfield of CC I guess it makes sense ... |
Thanks. sage: AA((-1)^(1/3))
/Users/kwankyu/GitHub/sage-dev/src/sage/symbolic/expression_conversions.py:212: DeprecationWarning: Conversion of negative number to fractional power to AA is deprecated
See https://github.com/sagemath/sage/issues/36735 for details.
return self.arithmetic(ex, operator)
-1Your deprecation warnings should point to this PR #38362.
Yes. That should also be done in this PR. Something like --- a/src/sage/rings/qqbar.py
+++ b/src/sage/rings/qqbar.py
@@ -6441,6 +6441,10 @@ class AlgebraicNumberPowQQAction(Action):
if rt is not None:
if x._descr._value < 0:
if S is AA:
+ if d % 2 != 0 and d != 1 and x.sign() < 0:
+ from sage.misc.superseded import deprecation
+ deprecation(38362, "Raising negative algebraic real number "
+ "will give primitive root in future.")
return AlgebraicReal(ANRational((-rt)**n))
else:
z = QQbar.zeta(2 * d)._pow_int(n) |
|
Please rebase to the develop branch. |
|
Please update the PR title and the description, as the deprecation warnings will point to this PR. To avoid confusion, you may keep the original description below the new one. |
user202729
changed the title
|
Actually there's one more sage: ZZ(-1)^(1/3)
(-1)^(1/3)
sage: ZZ(-1).nth_root(3)
-1Should we also give warning? (though there's a difference, Because of how the current code is implemented, Meanwhile: unlike what happens for Any idea why does the |
I don't think so. The doc of sage: ZZ(-1)^(1/3)
(-1)^(1/3)
sage: QQ(-1)^(1/3)
(-1)^(1/3)Anyway we are not "fix"-ing this. |
It seems that deprecation warnings are skipped in "TESTS" block... |
@mkoeppe ? |
It looks like you haven't formatted the expected output exactly as given in the example for sage/src/sage/misc/superseded.py Line 91 in e042294 Perhaps you get better results if you follow that formatting exactly? |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
In that case I think the cons of changing outweigh the pros of the change and then we're better off keeping the current behaviour for the sake of not disturbing user code and/or the annoyance of a deprecation message. Perhaps until someone finds a convincing reason to overcome the cons. @mezzarobba had some more spirited argument for changing rational (odd denominator) exponentiation behaviour on AA. |
|
The inconsistency has been repeatedly demonstrated in my comments here and in #38564. I think the inconsistency justifies changing the behavior. How one weighs solving inconsistency vs keeping the status quo depends on one's value system. Each one has his own preference. I can't change that. |
|
On Tue, 3 Sept 2024, 17:49 Kwankyu Lee, ***@***.***> wrote:
The inconsistency has been repeatedly demonstrated in my comments here and
in #38564 <#38564>. I think the
inconsistency justifies changing the behavior. How one weighs solving
inconsistency vs keeping the status quo depends on one's value system. Each
one has his own preference. I can't change that.
I don't think we all agree that there an inconsistency.
John
—
… Reply to this email directly, view it on GitHub
<#38362 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXU32N3CX244P6GCU56DV3ZUXLAHAVCNFSM6AAAAABK2Z4IAWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRWHA3DKMZRGU>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Yes. That is why we are voting now. I count your comment as -1 vote. |
|
There are two parts of the behavior here. One is People seems to be Note that not fixing that one will make the diagram non-commutative. |
|
Probably useful for people who try to make their minds up: The coercion system in sage is so eager that it coerces |
|
The coercion in powering was introduced by Jeroen Demeyer in 2017 #24247. |
|
I think certain examples and arguments here are a bit misleading for the discussion at hand. What I, and I assume other number theorists, care so much about is the case Any examples in pure python, or e.g. this example,
is about |
After deprecation, you may use |
I agree with this. When a is something algebraic (not a floating point in RR or CC) and I see a^(1/n) or a**(1/n) I interpret it is "a root of the polynomial X^n-a"; so if there is just one such root in a's parent (e.g. AA) then that's what I expect to see. |
|
Do you think the current behavior sage: AA(-1)^(1/4)
0.7071067811865475? + 0.7071067811865475?*Ishould be fixed? |
I agree, but the problem is that sage automatically coerces to floats, so implicitly QQ (for exponents) is imbued with the real metric. That means people can use AA-elements seamlessly in contexts where the continuity in the exponent is expected. To illustrate: The coercion framework is a little too eager. I think it should not just by default coerce into a common parent for exponentiation. It's rare that exponentiation designates a binary operation instead of an action. But that's such an invasive change that we'd need a rather convincing reason to implement that. (Jeroen's argument was: multiplication also often designates an action and there we try common parent as well. I'm not sure I'm convinced by that reasoning. Rings are rather central objects in mathematics) |
|
I am curious if number theorists are happy with sage: a = QQbar(1)
sage: (AA(-2) + a - a)^(1/3)
0.6299605249474365? + 1.091123635971722?*I
sage: AA(-2)^(1/3)
-1.259921049894873?I guess such a weird example cannot be constructed in sage without using |
|
I should not presume to speak for all number theorists, but it is likely that they would be happy that for a in any exact field or ring (including AA and QQbar, number fields, fine fields), we would only allow a**e or a^e for integer exponents e. That only uses the ring structure. For rational powers, which may not exist or be unique within the same ring or field, we can have methods like nth_root() or pow() as shortcuts to getting roots of polynomials like X^n-a or X^d-a^n (for rational exponent n/D). All the issues about choices of branch are relevant over CC (or RR as a subfield of CC perhaps) but not for exact fields. |
|
Mixing QQbar and AA in one expression is asking for 😵💫 |
For a happy way out, see https://groups.google.com/g/sage-devel/c/s81ieq2vpVo/m/GNw9w_jpAAAJ Summary: we embrace the inconsistency between analytical fields and exact (algebraic) fields as features of sage. |
I guess the problem here is again with the case of If something like this happened in my computations I would take it as a sign that I am doing something very stupid, so it would be good to be clearly alerted :-) |
|
apologies. I clicked the wrong button which "closed" the PR. I've reopened it. As far as I can see it's now restored to its original state, but let me know if there is something further I should do to restore. |
|
No problem. That happens :-) |
|
The tally is: negative votes: @JohnCremona @dimpase @hgranath Thank you for your participation in the discussion. It seems that algebraists like the current behavior of AA(-1)^(1/3) and think how other analytical fields behave as irrelevant. That is understandable. @user202729 Sorry for the outcome. It is wise that you try to solve your problem at hand in another PR #38606. I hope that number theorists here pay attention to the PR. |
|
I'm still on the fence and I think other people might change their opinion as well, depending on the further evidence that arises. I think the discussion here shows that the majority of the participants prefer a branch choice in the algebraic reals that does not extend the base field if possible, and this is also the documented behaviour for AA. That's for AA in isolation. Note that for interactive work or for intentional code designed specifically for a particular computation, one can freely choose to use The real complications from design choices around For instance, people may not have good control over whether an element gets created in If someone came up with a reason to evaluate, say, Puiseux polynomials (an expression like |
|
On Thu, Sep 5, 2024 at 6:15 PM nbruin ***@***.***> wrote:
For instance, people may not have good control over whether an element
gets created in AA or inQQbar: for a polynomial f=x^3+x+1 over QQ, the
command f.roots(QQbar) gives back one root that has parent AA and two
others that lie in QQbar.
Sounds like a bug in the roots method to me if that is indeed the case. But
maybe it has been fixed lately? In Sagemath 10.4 I get
sage: R.<x> = QQ[]
....: f = x^3 + x + 1
....: for r in f.roots(QQbar):
....: print(parent(r[0]))
....:
Algebraic Field
Algebraic Field
Algebraic Field
Message ID: ***@***.***>
… |
Previously, the behavior of
AAis inconsistent with most other cases:This will give a deprecation warning when the user attempt to rely on that behavior.
Previous description
Intended to fix #36735 , but now it doesn't.
This fix tries to be more conservative in how to converts symbolic expression of the form
a^b: it still first convertatoself.fieldthen raise the result to power ofb, but it checks ifa<0andbis a fraction then raise an error (I think the previously computed result in this case is never expected).It does not fix #12745 though (an alternative, as mentioned in #36942 , is to convert to QQbar first then convert it to AA, then this code wouldn't be necessary. What do you think of that alternative approach?)
📝 Checklist