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

Make Sage work with maxima 5.47 #35707

Merged
merged 14 commits into from
Jul 1, 2023
Merged

Conversation

antonio-rojas
Copy link
Contributor

Maxima 5.47 has been released. The main change that affects Sage is https://sourceforge.net/p/maxima/code/ci/f5ca00582c24cfedca53a664335f6c13e1e40cf9, which makes the maxima ecl library crash because the e-val variable is unassigned. We fix this by running the initialize-runtime-globals function on load.

The remaining issues are a few harmless test failures caused by numerical noise and output format changes (such as additional parentheses near minus signs)

src/doc/de/tutorial/interfaces.rst Outdated Show resolved Hide resolved
src/doc/de/tutorial/interfaces.rst Outdated Show resolved Hide resolved
src/doc/de/tutorial/interfaces.rst Outdated Show resolved Hide resolved
src/doc/de/tutorial/interfaces.rst Outdated Show resolved Hide resolved
src/doc/de/tutorial/tour_algebra.rst Outdated Show resolved Hide resolved
src/doc/de/tutorial/tour_algebra.rst Outdated Show resolved Hide resolved
src/sage/calculus/calculus.py Show resolved Hide resolved
src/sage/functions/bessel.py Outdated Show resolved Hide resolved
@tornaria
Copy link
Contributor

tornaria commented Jun 2, 2023

Just FTR, doctests pass for me with 5.47.0 (normal and long) with the current PR. My suggestions are merely cosmetic, and I wouldn't mind keeping it as it is (all these testing is horrible anyway).

Once we settle on something, I could try to run tests on 5.47.0 / 5.46.0 / 5.45.1, just to make sure it's ok.

@kiwifb
Copy link
Member

kiwifb commented Jun 2, 2023

If the appending of .sage() fix most of the brackets, appearing and disappearing then it is a much better solution. It makes the answers much clearer.

Another solution would be subtracting the know answer and simplify it to 0. That can only work for exact results of course.

@@ -974,7 +974,7 @@ def __init__(self):
sage: chebyshev_U(x, x)._sympy_()
chebyshevu(x, x)
sage: maxima(chebyshev_U(2,x, hold=True))
3*((-(8*(1-_SAGE_VAR_x))/3)+(4*(1-_SAGE_VAR_x)^2)/3+1)
3*(...-...(8*(1-_SAGE_VAR_x))/3)+(4*(1-_SAGE_VAR_x)^2)/3+1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sage: maxima(chebyshev_U(2,x, hold=True)).sage()
4*(x - 1)^2 + 8*x - 5

I don't know what this is trying to acomplish... I guess that maxima knows how to handle chebyshev_U(2, x).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's testing conversion to maxima, so I'm not sure converting back to sage the result makes much sense

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's supposed to be an example of the __init__ method. Why send it to maxima at all?

@tornaria
Copy link
Contributor

tornaria commented Jun 2, 2023

Summary: don't waste too much time in this... Maybe just put the "new correct" answer in the elliptic_e test (with the # not tested b/c old maxima is broken tag) just so that the thing that shows up in the documentation is correct (and the thing in the comment seems to be incorrect (???).

sage: A.eigenvectors()
[[[0,4],[3,1]],[[[1,0,0,-4],[0,1,0,-2],[0,0,1,-4/3]],[[1,2,3,4]]]]
sage: A.eigenvectors().sage()
[[[0, 4], [3, 1]], [[[1, 0, 0, -4], [0, 1, 0, -2], [0, 0, 1, -4/3]], [[1, 2, 3, 4]]]]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is difficult to test correctly because the eigenvalues, eigenvectors, and multiplicities are split across three lists and need to be matched up. The eigenvectors I would not expect to be unique anyway.

Lazy fix: switch it to a matrix with only nonnegative eigenvalues and a set of eigenvectors whose entries can be chosen integral and nonnegative.

@@ -277,8 +277,8 @@ Another approach is to use the interface with Maxima:

sage: A = maxima("matrix ([1, -4], [1, -1])")
sage: eig = A.eigenvectors()
sage: eig
[[[-sqrt(3)*%i,sqrt(3)*%i],[1,1]], [[[1,(sqrt(3)*%i+1)/4]],[[1,-(sqrt(3)*%i-1)/4]]]]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I commented on this one already, but don't see it. I don't know why we're telling people to use the low-level maxima interface to compute the eigenvalues of a 2x2 matrix in a linear algebra tutorial. Personally I would just delete it (and the surrounding explanation).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then the gap section immediately after that should be dropped too. I think we should leave any further doc cleanup for a follow up ticket.

Copy link
Contributor

@orlitzky orlitzky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, we shouldn't hold this up any longer. The first commit is pretty important. Thanks.

@@ -133,6 +133,7 @@
ecl_eval("(require 'maxima \"{}\")".format(MAXIMA_FAS))
else:
ecl_eval("(require 'maxima)")
ecl_eval("(maxima::initialize-runtime-globals)")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems this is unfixing #26968, as seen here: https://github.com/void-linux/void-packages/actions/runs/5366826557/jobs/9736459766?pr=44624#step:7:43488

Presumably, calling initialize-runtime-globals will run set-pathnames and fail without the workaround that is in the try block... Can this be moved after that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My suggestion:

--- a/src/sage/interfaces/maxima_lib.py
+++ b/src/sage/interfaces/maxima_lib.py
@@ -133,12 +133,11 @@ if MAXIMA_FAS:
     ecl_eval("(require 'maxima \"{}\")".format(MAXIMA_FAS))
 else:
     ecl_eval("(require 'maxima)")
-ecl_eval("(maxima::initialize-runtime-globals)")
 ecl_eval("(in-package :maxima)")
-ecl_eval("(setq $nolabels t))")
-ecl_eval("(defvar *MAXIMA-LANG-SUBDIR* NIL)")
 ecl_eval("(set-locale-subdir)")
 
+# this has to happen early so the workaround works
+# do not add other initialization before this block
 try:
     ecl_eval("(set-pathnames)")
 except RuntimeError:
@@ -155,6 +154,8 @@ except RuntimeError:
     # Call `(set-pathnames)` again to complete its job.
     ecl_eval("(set-pathnames)")
 
+ecl_eval("(initialize-runtime-globals)")
+ecl_eval("(setq $nolabels t))")
 ecl_eval("(defun add-lineinfo (x) x)")
 ecl_eval('(defun principal nil (cond ($noprincipal (diverg)) ((not pcprntd) (merror "Divergent Integral"))))')
 ecl_eval("(remprop 'mfactorial 'grind)")  # don't use ! for factorials (#11539)

Calling initialize-runtime-globals will run set-pathnames and be subject
to the issue described in sagemath#26968.

Thus the workaround introduced in sagemath#35195 has to be done before anything
that may call set-pathnames (e.g. initialize-runtime-globals).
@tornaria
Copy link
Contributor

Please merge antonio-rojas#2 into this PR before merging this PR in main.

@github-actions
Copy link

Documentation preview for this PR (built with commit 808ab47; changes) is ready! 🎉

@vbraun vbraun merged commit 29daacb into sagemath:develop Jul 1, 2023
@mkoeppe mkoeppe added this to the sage-10.1 milestone Jul 1, 2023
@antonio-rojas antonio-rojas deleted the maxima-5.47 branch July 2, 2023 10:36
dimpase added a commit to dimpase/sage that referenced this pull request Sep 2, 2024
followup to sagemath#35707 (make sage maxima 5.47-compatible)

fixes sagemath#38599
@dimpase dimpase mentioned this pull request Sep 2, 2024
5 tasks
dimpase added a commit to dimpase/sage that referenced this pull request Sep 3, 2024
followup to sagemath#35707 (make sage maxima 5.47-compatible)

fixes sagemath#38599
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 27, 2024
    
followup to sagemath#35707 (make sage maxima 5.47-compatible)

fixes sagemath#38599


### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#38601
Reported by: Dima Pasechnik
Reviewer(s): Kwankyu Lee
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 28, 2024
    
followup to sagemath#35707 (make sage maxima 5.47-compatible)

fixes sagemath#38599


### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#38601
Reported by: Dima Pasechnik
Reviewer(s): Kwankyu Lee
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants