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.

Sign up for GitHub

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

Jump to bottom

Tweak a few tests to pass when giac is not installed #38690

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Tweak a few tests to pass when giac is not installed #38690

wants to merge 3 commits into from

Conversation

orlitzky
Copy link
Contributor

This is one of the prerequisites to make it possible to install sage without giac (#38668). These few tests can be made to work easily without a # needs giac, so let's do that.

@orlitzky
src/sage/calculus/calculus.py: add "abs tol" to a numeric integral test
3668128 
Various implementations get it a little (1e-10) different.
@orlitzky
src/sage/symbolic/integration/integral.py: accept both giac/sympy ans…
1037153 
…wers

We have an integration test in this file that is looking for the giac
result, but sympy gives an equivalent one. We should accept that, too.
@orlitzky
src/sage/functions/piecewise.py: switch one test from giac to sympy
df6d309 
There's one piecewise test that uses algorithm='giac', but it will
soon be possible to install sage without giac. We could make the test
conditional on giac's presence, but the same thing works with sympy,
so let's just use that.
@orlitzky orlitzky mentioned this pull request Sep 21, 2024
Open
@github-actions GitHub Actions
Copy link

Documentation preview for this PR (built with commit df6d309; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@orlitzky
Copy link
Contributor Author

As always, the CI fails are bogus/unrelated.

dcoudert
dcoudert reviewed Sep 22, 2024
View reviewed changes
Copy link
Contributor

@dcoudert dcoudert left a comment

Choose a reason for hiding this comment

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

shouldn't you use annotations like # needs sage.libs.giac and/or # needs sympy ?

@orlitzky
Copy link
Contributor Author

There will be a PR where I add a bunch of "needs" tags, but in these few cases I was able to update the expected output to work both with/without giac, so a new "needs" tag isn't necessary. Sympy is a standard package and our "integrate" commands already use it unconditionally; so long as nobody adds a --disable-sympy flag, these tests should pass unconditionally because at least one of the two packages will be installed.

It's unrelated to this issue, but integrating with algorithm='giac' currently uses the slow giac pexpect interface and not the library interface in sage.libs.giac. I've changed this in #38686 to use the better interface, and that PR does add some # needs sage.libs.giac tags (because after the PR, it's using the library interface).

We're also missing a way to detect the giac pexpect interface, but I've added that in #38672. In theory you could have the executable installed but not the library... some binary distros have been known to split packages up like this.

dcoudert
dcoudert approved these changes Sep 22, 2024
View reviewed changes
Copy link
Contributor

@dcoudert dcoudert left a comment

Choose a reason for hiding this comment

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

thank you for the explanaition. LGTM.

@orlitzky
Copy link
Contributor Author

Sure, thanks for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcoudert dcoudert dcoudert approved these changes

Assignees
No one assigned
Labels
s: positive review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@orlitzky @dcoudert