New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make refine_embedding into a method of number fields instead of stand-alone #18836
Comments
Branch: u/cremona/18836-refine_embedding |
Commit: |
New commits:
|
comment:2
Hello John,
Could you make it more readable like
Vincent |
Reviewer: Vincent Delecroix |
comment:3
OK, I am working on it. On your second point I am not sure that it is so wasteful to find all roots of a polynomial in a real / complex field than to refine one approximate root to higher precision, but I will experiment to see if that is possible. And of course, this code (mostly by me) has been in Sage for a few years already, all I did here was to move it! |
comment:5
I did what was asked for (almost) and rebased onto 6.8.beta7. There does not seem to be a way to ask for one root in a field close to a given approximation, so it still finds all roots in the higher precision field -- but only the roots, it does not contruct all teh associated embeddings, just the closest one. I think the code is less obfuscated now too. There is still a problem,though: in testing I get two errors (from essentially the same test) in sage/schemes/elliptic_curves/ell_point.py and period_lattice.py. These occur when a complex embedding has been refined to infinite precision (i.e. to an embedding into QQbar), but an error is caused when an element of QQbar has its square root taken: its _value is an element of sage.rings.real_mpfi.RealIntervalFieldElement which is surely wrong -- it should be the corresponding complex type -- and does not like its argument being asked for. I have not been able to track this down, so I have not set the ticket to "needs review" as I know that these tests fail, but I am guessing that I have somehow stumbled across a bug which has nothing to do with this ticket. Help appreciated! |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:7
I have merged this with 6.10 to make it possible to continue with it. |
comment:8
This code was written while debugging the above. It runs fine unless you comment out the two lines near the end. I would say that this is a bug in QQbar.sqrt!
|
comment:9
The fact that
It could well be that this is really the intention (and there could be other places where the interval is set to be a real thing!), in which case the bug is indeed in Looking at it a little bit more, I think the error is in fact in
It's just that elements where that happens are usually filtered out earlier. So in this case,
So actually, on the first pass So I see two solutions: either ensure that val is put back into CIF or ensure that "argument extraction" is done appropriately in cases where "val" is in RIF. |
comment:10
That is good, I thought that you (Nils) would be able to help with this. I think we should fix this on #20068, which I made a dependency for this ticket, so I will copy your comments there. |
comment:11
Hello, I followed the comment from #18333. It would actually make sense for elements in
Though there is a lot of code shared between Vincent |
comment:12
Thanks, I did not know of the meta-ticket #18333. If this issue (now at #20064) can be resolved as part of that, then good, but I hope that a quick resolution will be possible since #20064 is a real bug while it looks as if many of the issues at #18333 are less urgent (though worth doing, certainly). |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:14
I am about to see if the fix at #20064 deals with the remaining issues. |
Dependencies: #20064 |
comment:15
Replying to @JohnCremona:
It does, and now has a positive review, so I will set that as a dependency of this and ask for another review of this one. |
comment:16
I got doctest failing for this one. sage -t --long src/sage/schemes/elliptic_curves/ell_point.py # 1 doctest failed |
comment:18
You are right, I thought #20064 was already merged, while it is not. Everything is fine now.
Also, it would be nice to note that, when the refinement is not unique, an "arbitrary" one is returned:
|
comment:20
I am going to close this as a "won't fix". There are unresolved issues, it provided nothing new, and when I tried (today) to rebase it and get tests to pass I found that it is not just a case of moving refine_embedding() from a stand-alone function to a method of number fields, since the function is also called for embeddings with domain QQ and various real and complex field types. It is not worth it. |
The stand-alone function {{{refine_embedding}} has been around for a while, right at the bottom of sage/rings/number_field/number_field.py, not imported into the global namespace and hence having to be imported whenever needed. More seriously, it is very easy for users and developers to miss its existence entirely! (Proof: today I learned that one of my students needed such a function and wrote his own since he did not know that I had written the existing function a few years ago!)
I am moving the function into the class NumberField, so that one can now say
instead of
There are only about 3 places in the Sage library where this is used, which need small changes.
Depends on #20064
Component: number fields
Author: John Cremona
Branch/Commit: u/cremona/18836-refine_embedding @
94a1d56
Reviewer: Vincent Delecroix
Issue created by migration from https://trac.sagemath.org/ticket/18836
The text was updated successfully, but these errors were encountered: