Fix coercion problem with Algebraic Real Field

[RuchitJagodara]

• Simplify the expression before converting it to algebraic number
• Added a doctest

This patch fixes #12745. With this commit, the coerce
issue of Algebraic Real Field is fixed. Now it is giving correct answer for below cases

sage: x = exp(2*I*pi/7) + exp(-2*I*pi/7)
sage: QQbar(x) in AA
True
sage: AA(x)
1.246979603717467?

📝 Checklist

• The title is concise, informative, and self-explanatory.
• The description explains in detail what this PR is about.
• I have linked a relevant issue or discussion.
• I have created tests covering the changes.

⌛ Dependencies

None

[RuchitJagodara]

@mkoeppe, can you please review this PR? The PR is encountering test failures, but I believe the revised answer aligns closely with the expected one. So should I make adjustments to the tests?

[tscrim]

@videlec @zimmermann6 @nbruin @mezzarobba cc-ing some people who might be interested.

[videlec]

I do not think it is a good idea to systematically full_simplify. Can you test the following with your patch?

sage: %time AA(sum(QQbar(i).sqrt() for i in range(20)))
CPU times: user 35.5 ms, sys: 2.95 ms, total: 38.5 ms
Wall time: 47.1 ms
57.19384185642023?

[RuchitJagodara]

This is my output

sage: %time AA(sum(QQbar(i).sqrt() for i in range(20)))
CPU times: user 3.55 ms, sys: 0 ns, total: 3.55 ms
Wall time: 3.55 ms
57.19384185642023?

I didn't understand what you were thinking. Can you please clarify?

[tscrim]

You should give timings before your change and with your change to look at possible speed regressions.

Also, I would use %timeit instead of %time to remove some of the noise.

[nbruin]

I suspect you're going to be better off going via QQbar rather than via symbolics here. Symbolics will generally have rather high overhead. The example below illustrates this on the motivating example above.

sage: %timeit AA(x.simplify_full())
8.98 ms ± 34.5 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
sage: %timeit AA(QQbar(x))
382 µs ± 731 ns per loop (mean ± std. dev. of 7 runs, 1,000 loops each)

[RuchitJagodara]

So should I go with QQbar as I don't find any disadvantages of that (plus it is very fast) ?

[nbruin]

I would first explore QQbar solutions, indeed. However, as #12745 (comment) indicates, there are some more fundamental problems and inconsistencies between AA and QQbar (there is also the naming convention, since most books about algebraic numbers that use AA would use it to indicate the field of algebraic numbers; not just the real ones). So I expect that if you start making more conversions from SR to AA go through QQbar, you'll start to see side-effects: conversions that already worked previously will yield different results due to differences in branch cuts chosen between AA and QQbar.

[RuchitJagodara]

Did you find anything, @nbruin, or should I proceed with QQbar and then address any errors that may arise as a result? Alternatively, should I stick with simplify_full() for now, prioritizing correctness over speed? It's better to provide a correct answer than to deliver an incorrect one quickly.

[github-actions]

Documentation preview for this PR (built with commit 108470b; changes) is ready! 🎉

[tscrim]

Did you find anything, @nbruin, or should I proceed with QQbar and then address any errors that may arise as a result? Alternatively, should I stick with simplify_full() for now, prioritizing correctness over speed? It's better to provide a correct answer than to deliver an incorrect one quickly.

That is a fallacy. You could cause code to be so slow that it is rendered unusable. In the case at hand, it doesn't "know" (in general) that the result lies in AA (welcome to dealing with symbolics and other places where undecidable problems exist). However, we have no way to really explain that to the user in the output, so it defaults to the "safe" choice of saying no.

Also @nbruin's suggestion was for you to do the exploration.

That being said, looking at AA._element_constructor_, it seems to be trying to convert the result on each term rather than the final result:

sage: x.imag().is_zero()
True

The quick solution is likely to go through QQbar since it is algebraic closed and then convert that to AA, as opposed to trying to rework the converter to only look at the final result.

[RuchitJagodara]

I think this PR is merged by mistake (Because there was no positive review on this PR), and one problem is that I didn't fixed the answers of some doctests which were failing (because I was exploring QQbar solutions), so some doctests are failing, now. like below

sage: elems = [sqrt(5), 2^(1/3)+sqrt(3)*I, 3/4]
....: nf, nums, hom = number_field_elements_from_algebraics(elems,
....:                                                     embedded=True)
sage: list(map(hom, nums)) == list(map(QQbar, nums))
True
sage: list(map(hom, nums)) == elems
---------------------------------------------------------------------------
NotImplementedError                       Traceback (most recent call last)
...
NotImplementedError: cannot find radical expression

Above example was working fine before these changes but now it is giving the error.

And while exploring QQbar solutions, I observed that using QQbar instead of simplify_full may produce some problems as @nbruin said, if we are doing this conversion then we will face many side-effects and to fix them we have to check all functions which are related to QQbar and AA.

So what should I do, now ? Should I make a new PR/issue to fix these doctests or should I revert these changes?

[tscrim]

@vbraun This was indeed incorrectly merged into the develop branch. I don't recall anyone approving this either. Please revert the merge.

Perhaps there is an issue with the merge scripts?

[vbraun]

I didn't merge this PR, but this PR branch was entirely contained in #37038. The latter had positive review and was merged.

~/Sage$ git log --graph --oneline 55c9f9c5561
*   55c9f9c5561 gh-37038: Add minimal_normal_subgroups and maximal_normal_subgroups functions for permutation groups
|\  
| *   fa26be9ac37 Merge branch 'develop' into add_minimal_normal_subgroups_method
| |\  
| * | 0f17e65a28f Corrected the error in functions
| * | 14c225e2ac6 Corrected linting error
| * | fa5b61b22ac Changed the method for constructing subgroups
| * | d2f349e347e Corrected some errors
| * | 297e8b2e576 Implemented functions in ParentLibGAP
| * | df7bf349558 Improved the codestyle
| * | c1f9dbecc8d Changed the example and added some description
| * | 4c351f711ac Added another function maximal_normal_subgroups and did some changes in example
| * | 4fc4d26e180 Convert the result back into sage subgroup
| * | cc09a88a561 Update expression.pyx
| * | 295e3836840 Update qqbar.py
| * | fe0fd281b89 Added the function
| * |   108470bce0c Merge remote-tracking branch 'upstream/develop' into develop

There is nothing wrong with this PR, its still open. The merged note will just remind you that currently there is no new code here (i.e. merging would not add new code)

[tscrim]

@vbraun I see, thanks for the diagnosis and explanation. Do you think you could squash away all of those intermediate commits from the main branch? I don't think it is a good idea to have some WIP that was reverted because it was meant for an independent ticket. I can figure out the precise commits if that would be helpful.

@RuchitJagodara Please make sure your branches are properly independent (and only have correct dependencies and common history).

[vbraun]

The commit history is immutable, once its released its done. Don't worry, sometimes the commit history is messy. But that is not a problem, git has all the tools necessary to deal with it.

Really squashing can only be done before code is released, and by the author and not by the release manager.

[tscrim]

Okay, thank you for the explanation. I am slightly worried about if someone is doing a git bisect and finds these commits and thinks the problem is associated with this PR. Admittedly it is a highly unlikely scenario.

@RuchitJagodara You will need to fix the commits here by copying them to a new branch and force pushing it to this branch on git.

[RuchitJagodara]

@RuchitJagodara Please make sure your branches are properly independent (and only have correct dependencies and common history).

I will take care of that and apologize for my mistake.

@RuchitJagodara You will need to fix the commits here by copying them to a new branch and force pushing it to this branch on git.

I tried to force push the branch but it is giving me the following error

remote: Permission to sagemath/sage.git denied to RuchitJagodara.
fatal: unable to access 'https://github.com/sagemath/sage.git/': The requested URL returned error: 403```

[vbraun]

This PR is based on the develop branch in your own account https://github.com/RuchitJagodara/sage.git, you have to push there.

[RuchitJagodara]

I have pushed my changes there but it is not reflecting here.

[tscrim]

This PR might just be fully corrupted and we should close it and have @RuchitJagodara open a new one based on a branch that is not develop. We completely fragment the discussion, but it might be the only way to repair the broken git history with GH. Any thoughts @vbraun @mkoeppe @kwankyu?