Skip to content
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

internal side effect in roots? #9538

Closed
zimmermann6 opened this issue Jul 18, 2010 · 27 comments
Closed

internal side effect in roots? #9538

zimmermann6 opened this issue Jul 18, 2010 · 27 comments

Comments

@zimmermann6
Copy link

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???

CC: @nbruin

Component: calculus

Keywords: roots, side effect

Author: Robert Dodier, Burcin Erocal

Reviewer: Paul Zimmermann

Merged: sage-4.6.alpha3

Issue created by migration from https://trac.sagemath.org/ticket/9538

@mwhansen
Copy link
Contributor

comment:1

I'm pretty sure this is just #8734.

@zimmermann6
Copy link
Author

comment:2

Replying to @mwhansen:

I'm pretty sure this is just #8734.

I tried the patch coming with #8734, but this does not resolve this issue.

Paul

@qed777
Copy link
Mannequin

qed777 mannequin commented Aug 12, 2010

comment:3

Replying to @zimmermann6:

Replying to @mwhansen:

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)]

@burcin
Copy link

burcin commented Sep 26, 2010

comment:4

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 #8734 comment:3:

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?

@burcin burcin added this to the sage-4.6 milestone Sep 26, 2010
@zimmermann6
Copy link
Author

comment:5

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

@burcin
Copy link

burcin commented Sep 26, 2010

Attachment: trac_9538-maxima_kill.patch.gz

@burcin
Copy link

burcin commented Sep 26, 2010

comment:6

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.

@burcin
Copy link

burcin commented Sep 26, 2010

Author: Robert Dodier, Burcin Erocal

@zimmermann6
Copy link
Author

comment:7

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

@burcin
Copy link

burcin commented Sep 26, 2010

patch for maxima spkg

@burcin
Copy link

burcin commented Sep 26, 2010

comment:8

Attachment: maxima_package.patch.gz

Replying to @zimmermann6:

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.patchIt 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.

@zimmermann6
Copy link
Author

comment:9

maybe we should wait that #8645 is fixed and merged within Sage to review that ticket.

Paul

@qed777
Copy link
Mannequin

qed777 mannequin commented Sep 30, 2010

comment:10

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.

@qed777
Copy link
Mannequin

qed777 mannequin commented Sep 30, 2010

comment:11

Replying to @zimmermann6:

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?

@qed777
Copy link
Mannequin

qed777 mannequin commented Sep 30, 2010

comment:12

Nils, do you have any thoughts about the problem in comment 7ff?

@zimmermann6
Copy link
Author

comment:13

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

@zimmermann6
Copy link
Author

comment:14

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

@qed777
Copy link
Mannequin

qed777 mannequin commented Oct 1, 2010

comment:15

Replying to @zimmermann6:

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.

@zimmermann6
Copy link
Author

comment:16

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

@burcin
Copy link

burcin commented Oct 1, 2010

apply only this patch -- forget about the package :)

@burcin
Copy link

burcin commented Oct 1, 2010

comment:17

Attachment: trac_9538-maxima_kill.take2.patch.gz

I uploaded an alternate patch, attachment: trac_9538-maxima_kill.take2.patchThis issues the kill command Robert Dodier recommended when initializing maxima. There is no need for any changes to the maxima package.

@zimmermann6
Copy link
Author

Reviewer: Paul Zimmermann

@zimmermann6
Copy link
Author

comment:18

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?

@burcin
Copy link

burcin commented Oct 3, 2010

comment:19

Thank you for the review.

Replying to @zimmermann6:

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.

@zimmermann6
Copy link
Author

comment:20

If it is fixed upstream, this doctest should fail: [...]

excellent!

Paul

@qed777
Copy link
Mannequin

qed777 mannequin commented Oct 4, 2010

Merged: sage-4.6.alpha3

@qed777 qed777 mannequin removed the s: positive review label Oct 4, 2010
@qed777 qed777 mannequin closed this as completed Oct 4, 2010
@vbraun
Copy link
Member

vbraun commented Oct 29, 2010

comment:22

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants