-
-
Notifications
You must be signed in to change notification settings - Fork 481
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
move number fields to new coercion, implement embeddings #4276
Comments
comment:1
I'll actually upload the patches when I'm on a stable enough connection to do so... |
Attachment: 4276-nf-coerce1.patch.gz |
Attachment: 4276-nf-coerce2.patch.gz |
Attachment: 4276-nf-coerce3.patch.gz |
comment:2
Attachment: 4276-nf-coerce4.patch.gz To help possible reviewers of what looks like very useful and good stuff, would it be possible to write a few words about what is going on here? By the way, the second patch does not view properly whan I click on it. I don't know why. |
comment:3
Yes, I explained this to some people at Sage Days 10, but it certainly could use some explanation here. These patches move coercion over to the new api, which is (hopefully) easier to understand and use as well as being faster. As part of this move, we also get the benefit of being able to specify embeddings at creation time, into RR or CC being the most common. (The embeddings into RR and CC are by default into "lazy" fields so the path can be followed to a field of any precision.) Cyclotomic fields and fields created with the "QuadraticField" command come with their standard embeddings. A field with an embedding can do arithmetic with its ambient field, and if two number fields have an embedding into a common field than elements can be moved from one to the other as well. Here is a brief example (though more can be found in the documentation):
This also paves the path for more sophisticated arithmetic, like automatic compositums. |
comment:4
I downloaded these patches not so much in order to review them but because I wanted to see if they would conflict with any changes I might make in order to fix #4193. Anyway, they apply and build fine (eventually) under 3.1.4. Testing number_field/ passes except for a minor numerical noise issue:
But I can't really do much to test the new functionality, since I'm not sure what it's supposed to do. You say above "here is a brief example (though more can be found in the documentation)" but I can't see any; all of the various number field constructors now accept an embedding parameter but there's no explanation or tests in the docstrings. Can we have a bit of demystification? |
comment:5
Thank you for taking a look at this. I really don't want these patches to bitrot. I will try to post another patch with more docstrings. Essentially, the embedding is a specified coercion into another field (typically RR or CC, though embeddings to any other field can be specified). This should make what you're trying to do at #4193 trivial (or at least very easy). The embedding parameter specifies where the generator of the number field should land (the closest root being taken if the root is not exact). So by doing
I am saying I want the number filed defined by |
comment:6
Attachment: 4276-nf-embedding-docs.patch.gz OK, I've attached some more documentation on embeddings. Also, since this is a coercion ticket, one of the main points of testing it is to make sure things still work like they used to. |
comment:7
Thanks for clarifying things there. Your new docstrings assert that cyclotomic fields all have their usual embeddings so coercion between them should work, but I'm slightly concerned by the following non-associativity of addition:
This problem is not in any way new, and existed before your patches; but shouldn't your patches fix it? |
comment:8
No, my patch does not automatically compute compositums, just coercions one direction or the other. This should be done, is on my (long) todo list, and of course is especially easy for cyclotomic fields, but this ticket is big enough already... Note that Sage does not guarantee associativity of addition. Consider
This is because there is an ambiguous ordering of the variables if the rightmost addition is performed first. What should be true, however, is if both operations succeed then they should give equal answers. |
comment:10
I am trying to referee this, but it's going to take some time. I'm going to put it into my tree and try to move some of my code that does this by hand to the embedding architecture. Email me in a month. I have some questions and problems: I really need to be able to specify embeddings after creation, because tons of functions create number fields (composite_fields, for example, or subfields) and I really don't want to specify all possible embeddings of the results... First, why squarefree_part bounded? This could be a separate patch. Second, I can specify non-embeddings that seem to be ignored:
Finally, I am having lots of trouble creating non-default embeddings:
And even worse, non-embeddings. check the sign of sqrt(2) below)
|
comment:11
Attachment: 4276-ncalexan.patch.gz
|
comment:12
Thanks very much, ncalexan, for taking over refereeing of this patch. I don't have the background to review something like this (my initial attempts above made me realise that) and I was worried that other more qualified people would now assume I had it under control. |
comment:13
Yes, in retrospect it could be. I moved that code out of the number field file, and while doing so decided to optimize it.
I'm not sure what you're trying to show with your example. It picks the closest root (so one doesn't need to specify the root to an arbitrary precision).
I agree these tracebacks are bad, but it's having trouble deciding which of Isqrt(2) or -Isqrt(2) is closest to sqrt(2). I'll have it throw a more sensible error instead. |
Attachment: 4276-nf-coerce-all.patch.gz Everything rebased against 3.2.1 |
comment:14
I've rebased against 3.2.1. Still building, but I'm off to bed. |
apply after the latest rebase |
comment:16
Attachment: 4276-nf-coerce-fixes.patch.gz The attached patch should address the issues. |
comment:17
4276-nf-coerce-fixes.patch does also fix the doctest failure for me, so we finally have no more doctest failures with this and the two patches from #3623 applied. Cheers, Michael |
comment:18
This code is good, it really is, but it's just not tested. I don't have time right now to test it and I will be sans computer over the Christmas break. I have to give this a negative review because I do not believe the embeddings work in anything more than the trivial situation: two absolute fields, compatible embeddings into CC. On the other hand, I have no reservations about the coercion part of this patch. Are we willing to accept almost certainly broken new functionality to have valuable infrastructure upgrades?
|
comment:19
I see now that I typoed in the last stanza. Try this instead:
|
Attachment: 4276-nf-coerce-fixes2.patch.gz apply on top of the others |
comment:20
Replying to @ncalexan: Hi Nick,
Ok.
I would say the answer is to merge this as is and then open follow up tickets for the problems. Does #4692 address your current concern? It looks like 3.2.2 will be longer than I have though, so getting this in now should give us the time needed to straighten this out before release. Thoughts? Cheers, Michael |
comment:21
I think even the "most trivial" embeddings of absolute fields into CC will be very useful, and a step forward. As this is the first real use of embeddings in the coercion model, I worked on coercion and embeddings at the same time. I don't embeddings are "almost certainly broken", but keep trying to break it. Not doing everything one can hope for is acceptable to me, as long as it fails gracefully. And it doesn't break currently working code. And thanks again for taking the time to look at this and try it out, it's a huge patch. |
comment:22
Provided that the doctests pass, that known issues have a ticket, and that the default behaviour (without specifying any embedding) still works as in the past, then we should not hold this back. Like Nick, I do not have time to test this thoroughly, but someone needs to stick their neck out with a positive review! As far as I can see Nick has done most one this, and mabshoff at least has checked the doctests, which should be enough. |
comment:23
For the record:
are in Sage 3.2.2.alpha0 on probation since a formal review is still outstanding. Cheers, Michael |
This comment has been minimized.
This comment has been minimized.
comment:25
Comment by Nick Alexander, but it ended up in the wrong box, so I am appending it here: I'm not sure what is supposed to happen, but this probably isn't it. AFAICT, these embeddings are not compatible. Or they are only after some work, that makes the results at the end strange. Please explain why this is correct if it is!
|
This comment has been minimized.
This comment has been minimized.
comment:26
Yes, that is a bug. I wasn't considering all the roots in the ambient field, which I now see is necessary. |
apply on top of fixes2 |
comment:27
Attachment: 4276-nf-coerce-fixes3.patch.gz OK, the issue found by Nick has been resolved. |
comment:28
For the record: With 4276-nf-coerce-fixes3.patch applied on top of the other patches and my 3.2.2.alpha1 merge tree all doctests pass. Cheers, Michael |
comment:29
I just discovered populate_coercion_lists(embedding=), and I am now a lot happier. (Still not thrilled, because it caches aggressively and if you do any arithmetic before changing the embedding it might not take.) This is what I want to be able to do: (The composite fields call needs new code of mine, to be posted shortly.)
I also want to assert an embedding of L into CLF and have the coercion model discover that I can convert Q1 -> L -> CLF and Q2 -> L -> CLF. Then we'll be getting somewhere. |
comment:30
After a chat in IRC with Nick I have merged 4276-nf-coerce-fixes3.patch in Sage 3.2.2.alpha1 Cheers, Michael |
comment:31
Nick, I'd like to see that happen too--but I can live with that type error for now. The caching is so that it doesn't need to apply the coercion logic every time an arithmetic operation is performed. CC me on the ticket when you get your composite_fields code up. Robert |
comment:32
Replying to @robertwb:
Hi, I would greatly prefer that now with fixes3 merged on top of the previous patches we could get this ticket (positively) reviewed and address any followup issues or Nick's improvements on a new ticket. That way the credit situation is much cleaner and this ticket isn't left to hang open. Cheers, Michael |
comment:33
Yes, I'm thinking a separate ticket too. This is a monstrously huge ticket already. |
comment:34
I am giving this a positive review in Nick's name (sue me :)), so that it can be officially merge. Cheers, Michael |
comment:35
Merged
in Sage 3.2.2.rc0. Plese open followup tickets for new issues/improvements on top of these patches. Cheers, Michael |
comment:36
Merged in Sage 3.2.2.rc0. |
comment:37
Nick: Obviously if anything goes wrong I am the one to blame here :) Cheers, Michael |
comment:38
Thanks. I think Cremona's comment hits the nail on the head--a lot of people have looked at this code but no one was willing to stick their neck out and give a positive review. I feel confident that this code will not degrade behavior or give wrong answers. I am also confident that it doesn't do everything that one would want to do, but I consider the latter additional enhancements, not defects in this ticket. |
comment:39
That is fine with me. I am confidently hoping that everything I used to do with number fields will still work, with no noticeable performance cost, and that some new things are possible which used not to be. Do the docstrings for the new code contain some hint of the new capabilities? As the number of patches has grown rather it is not so easy to tell... Onward and upward! |
comment:40
Yes, there is documentation and examples. |
See summary.
CC: @craigcitro @williamstein
Component: coercion
Issue created by migration from https://trac.sagemath.org/ticket/4276
The text was updated successfully, but these errors were encountered: