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
[three cheers] implementation of QQbar #1275
Comments
Attachment: qqbar.hg.gz Attachment: 7427-for-review.patch.gz |
comment:1
I just read through this code.
sage: hg_sage.apply('qqbar.hg')
|
comment:2
I just read through this code.
|
Attachment: qqbar-full.hg.gz |
comment:3
OK, I tried to be excessively clever with Mercurial here, and it backfired. The patch qqbar.hg evidently only works if you start with the exact same version of Sage that I started with, then apply patches from #1270 and #1273 on that version, then apply qqbar.hg. I've attached qqbar-full.hg, which is a single bundle containing #1270+#1273+qqbar.hg; you should be able to apply this on any sufficiently new version of Sage, instead of requiring the exact same version I had. |
comment:5
I have been testing/playing with this and read much of the code and it looks very nice. Thanks! One thing that looked really out of place to me was the handling of the golden ratio constant. I'm attaching a patch that I would like you to consider as an alternative way of doing it. |
Attachment: qqbar-sqrt-golden.patch.gz |
comment:6
I merged qqbar-full.hg, but I am waiting for comment on qqbar-sqrt-golden.patch submitted by Robert. Excellent! Cheers, Michael |
comment:7
Robert, my golden ratio code has the "advantage" that computations like
If you don't think that's useful, then I'm happy to switch the implementation. I'm puzzled by your new chunk in .argument(); as far as I can tell, it should be redundant. (Did you upgrade to the MPFI spkg in #1268 before applying #1270? That version fixes some bugs that cause problems in .argument(). Note that if you apply #1270 and rebuild, then upgrade #1268, it has no effect; if you did things in that order, you would also need to touch mpfi.pxi, like it says in the description for #1268.) Also, sqrt() needs a scary warning about branch cuts, along the lines of the warning in argument(). (Now that I look, I see that Thanks for the review! |
comment:8
I wasn't able to install the MPFI spkg, hence the new chunk in argument. If that makes it redundant, than we don't need it. In fact, I thought this is why you did golden_ration the way you did ('cause without the argument "fix" the imaginary part was not exactly 0). I agree both sqrt() and Here is a question. In your way, in your original version, if you do
is the answer exact? In any case, this should definitely get included and we can deal with this issue later. |
comment:9
Merged in 2.8.15.alpha2. |
The attached qqbar.hg bundle provides an implementation of QQbar (the algebraic closure of the rationals), with an embedding into CC. (The embedding is built-in, so there's no version without the embedding.)
testall passes on both 32-bit and 64-bit x86 Linux.
The bundle requires the new MPFI spkg from #1268, and the patches from #1270 and #1273.
The bundle contains two patches. The first has all the actual functionality; the second only handles the file rename from algebraic_real.py to qqbar.py. I'm going to also attach a copy of this first patch, for review purposes; but it should not be applied separately--apply the bundle instead.
Component: misc
Issue created by migration from https://trac.sagemath.org/ticket/1275
The text was updated successfully, but these errors were encountered: