Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#9538 closed defect (fixed)

internal side effect in roots?

Reported by: zimmerma Owned by: burcin
Priority: blocker Milestone: sage-4.6
Component: calculus Keywords: roots, side effect
Cc: nbruin 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)

trac_9538-maxima_kill.patch (1.0 KB) - added by burcin 4 years ago.
maxima_package.patch (4.8 KB) - added by burcin 4 years ago.
patch for maxima spkg
trac_9538-maxima_kill.take2.patch (2.0 KB) - added by burcin 4 years ago.
apply only this patch -- forget about the package :)

Download all attachments as: .zip

Change History (25)

comment:1 follow-up: Changed 4 years ago by mhansen

I'm pretty sure this is just #8734.

comment:2 in reply to: ↑ 1 ; follow-up: Changed 4 years ago by 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.

Paul

comment:3 in reply to: ↑ 2 Changed 4 years ago by mpatel

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 4 years ago by burcin

  • Milestone set to 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 4 years ago by zimmerma

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 4 years ago by burcin

comment:6 Changed 4 years ago by burcin

  • Authors set to Robert Dodier, Burcin Erocal
  • Status changed from new to 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: Changed 4 years ago by zimmerma

  • Status changed from needs_review to 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

Changed 4 years ago by burcin

patch for maxima spkg

comment:8 in reply to: ↑ 7 Changed 4 years ago by burcin

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 4 years ago by zimmerma

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

Paul

comment:10 Changed 4 years ago by mpatel

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 in reply to: ↑ 7 Changed 4 years ago by mpatel

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 4 years ago by mpatel

  • Cc nbruin added

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

comment:13 Changed 4 years ago by zimmerma

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: Changed 4 years ago by 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.

Paul

comment:15 in reply to: ↑ 14 Changed 4 years ago by mpatel

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 4 years ago by zimmerma

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 4 years ago by burcin

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

comment:17 Changed 4 years ago by burcin

  • Status changed from needs_info to 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: Changed 4 years ago by zimmerma

  • Reviewers set to Paul Zimmermann
  • Status changed from needs_review to 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 in reply to: ↑ 18 Changed 4 years ago by burcin

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 4 years ago by zimmerma

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

excellent!

Paul

comment:21 Changed 4 years ago by mpatel

  • Merged in set to sage-4.6.alpha3
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:22 Changed 4 years ago by vbraun

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.

Note: See TracTickets for help on using tickets.