#9538 closed defect (fixed)
internal side effect in roots?
Reported by: | Paul Zimmermann | Owned by: | Burcin Erocal |
---|---|---|---|
Priority: | blocker | Milestone: | sage-4.6 |
Component: | calculus | Keywords: | roots, side effect |
Cc: | Nils Bruin | Merged in: | sage-4.6.alpha3 |
Authors: | Robert Dodier, Burcin Erocal | Reviewers: | Paul Zimmermann |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
Consider the following with Sage 4.4.4:
sage: var('a6,a5,a4,x') (a6, a5, a4, x) sage: e=15*a6*x^2 + 5*a5*x + a4 sage: e.roots(x) [(-1/30*(sqrt(-12*a4*a6 + 5*a5^2)*sqrt(5) + 5*a5)/a6, 1), (1/30*(sqrt(-12*a4*a6 + 5*a5^2)*sqrt(5) - 5*a5)/a6, 1)]
This is fine. However:
sage: var('f6,f5,f4,x') (f6, f5, f4, x) sage: e=15*f6*x^2 + 5*f5*x + f4 sage: e.roots(x) [(1/30*(-I*sqrt(35) - 5)/binomial(n, k), 1), (1/30*(I*sqrt(35) - 5)/binomial(n, k), 1)]
WTF???
Attachments (3)
Change History (25)
comment:1 follow-up: 2 Changed 12 years ago by
comment:2 follow-up: 3 Changed 12 years ago by
comment:3 Changed 12 years ago by
Replying to zimmerma:
Replying to mhansen:
I'm pretty sure this is just #8734.
I tried the patch coming with #8734, but this does not resolve this issue.
Could you try this again? After applying #8734 to 4.5.3.alpha0 (and ignoring the patch rejects), I get
sage: var('f6,f5,f4,x') (f6, f5, f4, x) sage: e=15*f6*x^2 + 5*f5*x + f4 sage: e.roots(x) [(-1/30*(sqrt(-12*f4*f6 + 5*f5^2)*sqrt(5) + 5*f5)/f6, 1), (1/30*(sqrt(-12*f4*f6 + 5*f5^2)*sqrt(5) - 5*f5)/f6, 1)]
comment:4 Changed 12 years ago by
Milestone: | → sage-4.6 |
---|
The patch attached to #8734 indeed fixes this problem. However there are simpler solutions, and I'm not convinced that #8734 is necessary, given that #7377 could solve this in a much cleaner way. Also see the discussion linked from comment:3:ticket:8734:
http://groups.google.com/group/sage-devel/browse_thread/thread/67f0a63d00b8d835/06557a921a582f87
In particular, Robert Dodier's suggestion to apply this patch to maxima:
--- share/contrib/Zeilberger/testZeilberger.mac 9 Feb 2007 22:32:34 -0000 1.4 +++ share/contrib/Zeilberger/testZeilberger.mac 15 Jan 2010 19:10:53 -0000 @@ -110,3 +110,8 @@ FULL_TEST : append(GOSPER_TEST,EASY_TEST, HARD_TEST,EXTREME_TEST); + +kill (g1, g2, g3, g4, g5, g6, g7, + f1, f2, f3, f4, f5, f6, f7, f8, f9, f10, + h1, h2, h3, h4, h5, h6, h6b, h7, h8, h9, h10, h11, h12, h13, + d1, d2);
I can confirm that adding these lines to $SAGE_LOCAL/share/maxima/5.20.1/share/contrib/Zeilberger/testZeilberger.mac
solves this issue.
Perhaps this patch is already in a more recent version of maxima?
comment:5 Changed 12 years ago by
I confirm that with the #8734 patch applied to 4.5.3 (and ignoring the patch reject) it works fine.
However I'm convinced by Burcin's argument. I am ready to review a patch based on this patch applied to Maxima.
Paul
Changed 12 years ago by
Attachment: | trac_9538-maxima_kill.patch added |
---|
comment:6 Changed 12 years ago by
Authors: | → Robert Dodier, Burcin Erocal |
---|---|
Status: | new → needs_review |
Here is a maxima package that patches the file share/contrib/Zeilberger/testZeilberger.mac
as suggested by Robert Dodier:
http://sage.math.washington.edu/home/burcin/maxima-5.20.1.p1.spkg
attachment:trac_9538-maxima_kill.patch adds a doctest to check the example given in the description.
comment:7 follow-ups: 8 11 Changed 12 years ago by
Status: | needs_review → needs_info |
---|
I tried sage -i /tmp/maxima-5.20.1.p1.spkg
but got:
... ;;; OPTIMIZE levels: Safety=2, Space=0, Speed=3, Debug=2 ;;; End of Pass 1. ;;; Note: Creating tag: "_eclFGkv5HJdeKquW_9rDWPWz" for #P"binary-ecl/maxima-package.o" ;;; Internal error: Unable to find include directory ; - Binary file binary-ecl/maxima-package.fas is old or does not exist. ; Compile (and load) source file /usr/local/sage-4.5.3/sage/spkg/build/maxima-5.20.1.p1/src/src/maxima-package.lisp instead? y ; - Should I bother you if this happens again? y ; - Compiling source file ; "/usr/local/sage-4.5.3/sage/spkg/build/maxima-5.20.1.p1/src/src/maxima-package.lisp" ;;; Compiling /usr/local/sage-4.5.3/sage/spkg/build/maxima-5.20.1.p1/src/src/maxima-package.lisp. ;;; OPTIMIZE levels: Safety=2, Space=0, Speed=3, Debug=2 ;;; End of Pass 1. ;;; Note: Creating tag: "_eclFGkv5HJdeKquW_IVMWPWz" for #P"binary-ecl/maxima-package.o" ;;; Internal error: Unable to find include directory ; - Loading binary file "binary-ecl/maxima-package.fas" An error occurred during initialization: Filesystem error with pathname #P"/usr/local/sage-4.5.3/sage/spkg/build/maxima-5.20.1.p1/src/src/binary-ecl/maxima-package.fas". Either 1) the file does not exist, or 2) we are not allow to access the file, or 3) the pathname points to a broken symbolic link.. make[1]: *** [binary-ecl/maxima] Error 1 make[1]: Leaving directory `/usr/local/sage-4.5.3/sage/spkg/build/maxima-5.20.1.p1/src/src' make: *** [all-recursive] Error 1 *********************************************************** Failed to make Maxima. ***********************************************************
Did I something wrong? How to install the patched spkg?
Paul
comment:8 Changed 12 years ago by
Replying to zimmerma:
Did I something wrong? How to install the patched spkg?
I don't have any experience with the maxima spkg. I just applied the patch in attachment:maxima_package.patch. It works here, but just complains about not being able to build maxima.fasb
:
cp: cannot stat `maxima.fasb': No such file or directory
Perhaps someone more knowledgeable can help out. Building maxima as a library is #8645.
comment:9 Changed 12 years ago by
maybe we should wait that #8645 is fixed and merged within Sage to review that ticket.
Paul
comment:10 Changed 12 years ago by
Adding
kill (g1, g2, g3, g4, g5, g6, g7, f1, f2, f3, f4, f5, f6, f7, f8, f9, f10, h1, h2, h3, h4, h5, h6, h6b, h7, h8, h9, h10, h11, h12, h13, d1, d2);
directly to the bottom of
SAGE_ROOT/local/share/maxima/5.20.1/share/contrib/Zeilberger/testZeilberger.mac
without installing http://sage.math.washington.edu/home/burcin/maxima-5.20.1.p1.spkg fixes the problem in the description.
comment:11 Changed 12 years ago by
Replying to zimmerma:
I tried
sage -i /tmp/maxima-5.20.1.p1.spkg
but got:
[...]
Did I something wrong? How to install the patched spkg?
I have the same problem with the forthcoming 4.6.alpha2, if I've renamed SAGE_ROOT
since that copy of Sage was first compiled (and another installation with the previous name does not exist). In this case, even reinstalling the included Maxima, with
./sage -f spkg/standard/maxima-5.20.1.p0.spkg
triggers the behavior. Paul, did you happen to move or rename your Sage root directory? Does Burcin's p1 package install successfully and fix the roots problem with a Sage that has not been moved?
comment:12 Changed 12 years ago by
Cc: | Nils Bruin added |
---|
Nils, do you have any thoughts about the problem in comment 7ff?
comment:13 Changed 12 years ago by
Paul, did you happen to move or rename your Sage root directory?
no, I did all my experiments in /tmp/sage-4.6.alpha1, where I built sage-4.6.alpha1 from source.
I have no SAGE_ROOT
environment variable.
Paul
comment:14 follow-up: 15 Changed 12 years ago by
Burcin, do you agree that we try Mitesh's solution from comment 10, which avoids installing a new Maxima spkg? If so, if someone can provide a patch, I will review it.
Paul
comment:15 Changed 12 years ago by
Replying to zimmerma:
Burcin, do you agree that we try Mitesh's solution from comment 10, which avoids installing a new Maxima spkg? If so, if someone can provide a patch, I will review it.
Except for SAGE_LOCAL/bin
, the files under SAGE_LOCAL
are not under version control. I was mainly trying to check whether Robert's fix works with an already installed Maxima in Sage.
So far, it seems the build problem is orthogonal to the problem in the description.
comment:16 Changed 12 years ago by
So far, it seems the build problem is orthogonal to the problem in the description.
agreed, but how can we proceed in practice so that I can review this ticket?
Paul
Changed 12 years ago by
Attachment: | trac_9538-maxima_kill.take2.patch added |
---|
apply only this patch -- forget about the package :)
comment:17 Changed 12 years ago by
Status: | needs_info → needs_review |
---|
I uploaded an alternate patch, attachment:trac_9538-maxima_kill.take2.patch. This issues the kill
command Robert Dodier recommended when initializing maxima. There is no need for any changes to the maxima package.
comment:18 follow-up: 19 Changed 12 years ago by
Reviewers: | → Paul Zimmermann |
---|---|
Status: | needs_review → positive_review |
ok for the last patch, which fixes the problem, and all tests pass (tested with Sage 4.4.4).
Paul
PS: a minor remark, is there a mechanism to update the calculus.py patch once the problem is fixed upstream?
comment:19 Changed 12 years ago by
Thank you for the review.
Replying to zimmerma:
PS: a minor remark, is there a mechanism to update the calculus.py patch once the problem is fixed upstream?
If it is fixed upstream, this doctest should fail:
sage: maxima = Maxima(init_code = ['load(simplify_sum)']) sage: maxima('f1') binomial(n,k)
Then we can remove the line with the kill
command.
comment:20 Changed 12 years ago by
If it is fixed upstream, this doctest should fail: [...]
excellent!
Paul
comment:21 Changed 12 years ago by
Merged in: | → sage-4.6.alpha3 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
comment:22 Changed 12 years ago by
For the record, this fails with maxima-5.22.1:
File "/home/vbraun/opt/sage-4.6.rc0/devel/sage-main/sage/calculus/calculus.py", line 368: sage: maxima('f1') Expected: binomial(n,k) Got: f1
None of the patches from maxima_package.patch
are in the updated maxima-5.22.1 package. See #10187 for the updated spgk. I take it that this is the expected behaviour and no further patches to maxima are necessary.
I'm pretty sure this is just #8734.