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

Use the faster libgiac interface for "giac" integration #38686

Merged
merged 11 commits into from
Dec 8, 2024

Conversation

orlitzky
Copy link
Contributor

While working on #38668, I noticed that all of our examples that use giac for integration do so with algorithm='giac'. This uses the pexpect interface that is much slower (and simply sloppier) than the library interface that would be used with algorithm='libgiac'.

To fix this, I could just change those examples to algorithm='libgiac', but since the libgiac interface is better in every way, that seems kind of rude to me. Most users aren't going to know what lib-anything is, and if they just want to use giac, they're going to try algorithm='giac'. So instead this PR makes algorithm='giac' do the right thing, and use the better interface. I've left the algorithm='libgiac' alone for now to avoid breaking any code.

Afterwards I have removed the pexpect giac integrator entirely, because there's no reason to use it, and no other code in sagelib uses it.

Copy link

github-actions bot commented Sep 20, 2024

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

@orlitzky
Copy link
Contributor Author

AFAICS the CI failure is an unrelated timeout:

2024-09-20T15:19:09.1450225Z ----------------------------------------------------------------------
2024-09-20T15:19:09.1451853Z sage -t --long --random-seed=286735480429121101562228604801325644303 src/sage/data_structures/bounded_integer_sequences.pyx  # Timed out
2024-09-20T15:19:09.1453353Z ----------------------------------------------------------------------

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 29, 2024

This looks like a good idea, thanks.

@orlitzky
Copy link
Contributor Author

orlitzky commented Oct 2, 2024

I didn't see the label change, thanks for the review.

@orlitzky orlitzky mentioned this pull request Oct 2, 2024
2 tasks
@vbraun
Copy link
Member

vbraun commented Oct 4, 2024

I'm getting

sage -t --long --warn-long 45.2 --random-seed=123 src/sage/symbolic/integration/integral.py
**********************************************************************
File "src/sage/symbolic/integration/integral.py", line 64, in sage.symbolic.integration.integral.IndefiniteIntegral.__init__
Failed example:
    integrate(Ex, x, algorithm='giac')  # long time
Expected:
    4*(-2*x^(1/3) + 1)^(3/4) + 6*arctan((-2*x^(1/3) + 1)^(1/4)) - 3*log((-2*x^(1/3) + 1)^(1/4) + 1) + 3*log(abs((-2*x^(1/3) + 1)^(1/4) - 1))
Got:
    4*(-2*x^(1/3) + 1)^(3/4) + 6*arctan((-2*x^(1/3) + 1)^(1/4)) - 3*log(abs((-2*x^(1/3) + 1)^(1/4) + 1)) + 3*log(abs((-2*x^(1/3) + 1)^(1/4) - 1))
**********************************************************************
File "src/sage/symbolic/integration/integral.py", line 209, in sage.symbolic.integration.integral.DefiniteIntegral.__init__
Failed example:
    integral(1/max_symbolic(x, 1)**2, x, 0, oo, algorithm='giac')
Exception raised:
    Traceback (most recent call last):
      File "/home/release/Sage/src/sage/doctest/forker.py", line 714, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/release/Sage/src/sage/doctest/forker.py", line 1144, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.symbolic.integration.integral.DefiniteIntegral.__init__[4]>", line 1, in <module>
        integral(Integer(1)/max_symbolic(x, Integer(1))**Integer(2), x, Integer(0), oo, algorithm='giac')
      File "/home/release/Sage/src/sage/misc/functional.py", line 785, in integral
        return x.integral(*args, **kwds)
               ^^^^^^^^^^^^^^^^^^^^^^^^^
      File "sage/symbolic/expression.pyx", line 13222, in sage.symbolic.expression.Expression.integral
        return integral(self, *args, **kwds)
      File "/home/release/Sage/src/sage/symbolic/integration/integral.py", line 1073, in integrate
        return integrator(expression, v, a, b)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/home/release/Sage/src/sage/symbolic/integration/external.py", line 259, in libgiac_integrator
        result = libgiac.integrate(Pygen(expression), v, a, b)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "sage/libs/giac/giac.pyx", line 1934, in sage.libs.giac.giac.GiacFunction.__call__
        return Pygen.__call__(self, *args)
      File "sage/libs/giac/giac.pyx", line 1109, in sage.libs.giac.giac.Pygen.__call__
        raise
      File "sage/libs/giac/giac.pyx", line 1099, in sage.libs.giac.giac.Pygen.__call__
        result = self.gptr[0](right.gptr[0], context_ptr)
    RuntimeError: Unable to check sign: 1>Infinity
**********************************************************************
File "src/sage/symbolic/integration/integral.py", line 697, in sage.symbolic.integration.integral.integrate
Failed example:
    integrate(abs(cos(x)), x, 0, 2*pi, algorithm='giac')
Expected:
    4
Got:
    Warning, integration of abs or sign assumes constant sign by intervals (correct if the argument is real):
    Check [abs(cos(sageVARx))]
    Discontinuities at zeroes of cos(sageVARx) were not checked
    4
**********************************************************************

@orlitzky
Copy link
Contributor Author

orlitzky commented Oct 6, 2024

Expected:
4*(-2x^(1/3) + 1)^(3/4) + 6arctan((-2x^(1/3) + 1)^(1/4)) - 3log((-2x^(1/3) + 1)^(1/4) + 1) + 3log(abs((-2x^(1/3) + 1)^(1/4) - 1))
Got:
4
(-2x^(1/3) + 1)^(3/4) + 6arctan((-2x^(1/3) + 1)^(1/4)) - 3log(abs((-2x^(1/3) + 1)^(1/4) + 1)) + 3log(abs((-2*x^(1/3) + 1)^(1/4) - 1))

These are the same anwer. I updated the expected output.

Expected:
4
Got:
Warning, integration of abs or sign assumes constant sign by intervals (correct if the argument is real):
Check [abs(cos(sageVARx))]
Discontinuities at zeroes of cos(sageVARx) were not checked
4

Nothing too scary here. This doctest already looks for the warning output on the previous line, where integrate is used without an algorithm argument. I tweaked the description a bit and added some ....

File "src/sage/symbolic/integration/integral.py", line 209, in sage.symbolic.integration.integral.DefiniteIntegral.init
Failed example:
integral(1/max_symbolic(x, 1)**2, x, 0, oo, algorithm='giac')
...
RuntimeError: Unable to check sign: 1>Infinity

This one on the other hand is a treat. It's reproducible using libgiac_integrator, but not through integrate(), which (if maxima fails) calls libgiac_integrator with exactly the same arguments. My guess is hidden state within sage.libs.giac.

In any case, I was able to work around it. Don't ask me what the difference is, but giac likes +infinity better than Infinity. You get the latter passing Sage's infinity directly to libgiac, and the former by calling _giac_() on Sage's infinity. So I added a few lines to the integrator to do that when the endpoints are plus/minus infinity.

vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 23, 2024
    
Part of sagemath#38668. If it's going to
be possible to disable giac, we need to guard all of the tests that use
it with either `# needs giac` or `# needs sage.libs.giac`.

I think I've gotten them all. A crude way to test:

1. `git rm -r src/sage/libs/giac` and rebuild to disable sage.libs.giac
2. build sage, and then delete the giac executable to disable the
pexpect interface

If you do these one at a time, it should ensure that the correct tags
are used. (Typically, if giac is missing, neither sage.libs.giac nor the
giac executable will be present, making it very easy to mix up the
tags.)

For bonus points you can undelete `src/sage/libs/giac` after building
but before testing to make sure the "needs" tags in those files are
accurate.

### Dependencies:

* sagemath#38756
* sagemath#38686 (not strictly required,
but it adds a few "needs sage.libs.giac" tags of its own)
    
URL: sagemath#38770
Reported by: Michael Orlitzky
Reviewer(s): Tobias Diez
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 26, 2024
    
Part of sagemath#38668. If it's going to
be possible to disable giac, we need to guard all of the tests that use
it with either `# needs giac` or `# needs sage.libs.giac`.

I think I've gotten them all. A crude way to test:

1. `git rm -r src/sage/libs/giac` and rebuild to disable sage.libs.giac
2. build sage, and then delete the giac executable to disable the
pexpect interface

If you do these one at a time, it should ensure that the correct tags
are used. (Typically, if giac is missing, neither sage.libs.giac nor the
giac executable will be present, making it very easy to mix up the
tags.)

For bonus points you can undelete `src/sage/libs/giac` after building
but before testing to make sure the "needs" tags in those files are
accurate.

### Dependencies:

* sagemath#38756
* sagemath#38686 (not strictly required,
but it adds a few "needs sage.libs.giac" tags of its own)
    
URL: sagemath#38770
Reported by: Michael Orlitzky
Reviewer(s): Tobias Diez
mkoeppe added a commit to passagemath/passagemath that referenced this pull request Nov 4, 2024
Add `# needs sage.libs.giac`, from sagemath/sage#38686 by @orlitzky, cherry picked
@orlitzky orlitzky force-pushed the no-pexpect-giac-integration branch from 8db21a2 to 2be5a18 Compare November 13, 2024 02:11
@orlitzky
Copy link
Contributor Author

In any case, I was able to work around it. Don't ask me what the difference is, but giac likes +infinity better than Infinity. You get the latter passing Sage's infinity directly to libgiac, and the former by calling _giac_() on Sage's infinity. So I added a few lines to the integrator to do that when the endpoints are plus/minus infinity.

I went back and reworked this. Infinity in giac is unsigned infinity, and the conversion was being done incorrectly (from signed in sage to unsigned in giac). After adding another case to the Pygen constructor, everything is fine. No hacks needed in the integration routine.

Before this commit, signed Sage infinities would be converted to
unsigned Giac infinities:

    sage: libgiac(+Infinity)
    Infinity

This can mess up your integrals, if nothing else. To fix it, we add an
additional case to the Pygen constructor. Sage's AnInfinity class
already knows how to convert itself to a Giac-friendly string, and
Pygen already knows what to do with those strings -- we just need
to put the pieces together. A new test case is added as well.
…egration

The library interface to libgiac is much more efficient than the
pexpect one, but the name algorithm="giac" is more attractive
(especially to newcomers) than algorithm="libgiac". We should just
Do The Right Thing and use libgiac when "giac" is requested.
The pexpect integrator is unused now that the "giac" integrator uses
libgiac. We also add a few "# needs sage.libs.giac" tags to the tests
that use the libgiac integrator, because they obviously will fail when
libgiac is not there.
One example in this file integrates with algorithm='giac', which can
fail if sage.libs.giac is not available to perform the integration.
One example in this file integrates with algorithm='giac', which can
fail if sage.libs.giac is not available to perform the integration.
…tegration

A few examples in this file integrate with algorithm='giac', which can
fail if sage.libs.giac is not available to perform the integration.
There's one test in this file for an integral where the "libgiac"
algorithm prints a warning, but the old "giac" algorithm did not. Now
that algorithm="giac" uses libgiac, we expect the warning in that case
too.
One "giac" integral in this file has changed its output slightly now
that the "giac" algorithm uses libgiac as opposed to the pexpect
interface. The difference between the new and old expected outputs
full_simplify() to zero, so this one is nothing to worry about.
With the recent improvement to the handling of signed infinities in
libgiac, this warning about singular points has vanished.
A comment in this file about failing doctests is still partially true
(the doctests fail if you remove the workaround), but it mentions a
"fixme" that does not exist. We delete the nonsense part.
Now that libgiac is being used for "giac" integrals, one test is
printing out "No checks were made for singular points of
antiderivative -1/sageVARx for definite integration in [1,+infinity]"
where previously the test was silent. We now "..." the junk away.
@orlitzky orlitzky force-pushed the no-pexpect-giac-integration branch from 2be5a18 to 09572b1 Compare December 4, 2024 21:36
@orlitzky
Copy link
Contributor Author

orlitzky commented Dec 4, 2024

If someone has a moment, I lost my reviewer.

This adds the final set of # needs tags to make giac optional (using meson, for now), but at the same time it switches integrate(..., algorithm="giac") to use the faster libgiac interface. That should have been trivial but there was a bug in sage.libs.giac's handling of infinity that complicated things. That is now fixed too.

Copy link
Contributor

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

LGTM as far as I can judge.

@orlitzky
Copy link
Contributor Author

orlitzky commented Dec 5, 2024

Thanks, I might finally be able to pull sage.libs.giac out into a separate package now (if the GCC 15 bugs ever end).

vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 6, 2024
…tion

    
While working on sagemath#38668, I
noticed that all of our examples that use giac for integration do so
with `algorithm='giac'`. This uses the pexpect interface that is much
slower (and simply sloppier) than the library interface that would be
used with `algorithm='libgiac'`.

To fix this, I could just change those examples to
`algorithm='libgiac'`, but since the libgiac interface is better in
every way, that seems kind of rude to me. Most users aren't going to
know what lib-anything is, and if they just want to use giac, they're
going to try `algorithm='giac'`. So instead this PR makes
`algorithm='giac'` do the right thing, and use the better interface.
I've left the `algorithm='libgiac'` alone for now to avoid breaking any
code.

Afterwards I have removed the pexpect giac integrator entirely, because
there's no reason to use it, and no other code in sagelib uses it.
    
URL: sagemath#38686
Reported by: Michael Orlitzky
Reviewer(s): Dima Pasechnik, Tobias Diez
vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 8, 2024
…tion

    
While working on sagemath#38668, I
noticed that all of our examples that use giac for integration do so
with `algorithm='giac'`. This uses the pexpect interface that is much
slower (and simply sloppier) than the library interface that would be
used with `algorithm='libgiac'`.

To fix this, I could just change those examples to
`algorithm='libgiac'`, but since the libgiac interface is better in
every way, that seems kind of rude to me. Most users aren't going to
know what lib-anything is, and if they just want to use giac, they're
going to try `algorithm='giac'`. So instead this PR makes
`algorithm='giac'` do the right thing, and use the better interface.
I've left the `algorithm='libgiac'` alone for now to avoid breaking any
code.

Afterwards I have removed the pexpect giac integrator entirely, because
there's no reason to use it, and no other code in sagelib uses it.
    
URL: sagemath#38686
Reported by: Michael Orlitzky
Reviewer(s): Dima Pasechnik, Tobias Diez
@vbraun vbraun merged commit a0e2239 into sagemath:develop Dec 8, 2024
22 of 24 checks passed
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.

5 participants