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

fix a calculus doctest (giac, laplace, integration) #22833

Closed
fchapoton opened this issue Apr 19, 2017 · 55 comments
Closed

fix a calculus doctest (giac, laplace, integration) #22833

fchapoton opened this issue Apr 19, 2017 · 55 comments

Comments

@fchapoton
Copy link
Contributor

that currently prevents many patchbots to run smoothly

CC: @rwst @frederichan-IMJPRG

Component: calculus

Author: Frédéric Chapoton, Marcelo Forets

Branch/Commit: e2d74c6

Reviewer: Travis Scrimshaw, Steven Trogdon

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

@fchapoton fchapoton added this to the sage-8.0 milestone Apr 19, 2017
@fchapoton
Copy link
Contributor Author

Branch: u/chapoton/22833

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 19, 2017

Branch pushed to git repo; I updated commit sha1. New commits:

2186acafixing calculus doctest (giac)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 19, 2017

Commit: 2186aca

@tscrim
Copy link
Collaborator

tscrim commented Apr 19, 2017

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Apr 19, 2017

comment:3

I thought someone had already fixed this, but I guess not.

@mforets
Copy link
Mannequin

mforets mannequin commented Apr 20, 2017

comment:4

the error was because of the double :: in TETS or the Unable to parse Giac output? or both? and how did you know that integration is ok? (i still get the NotImplementedError in sage8.0beta2) because the "dummy integrate" keyword of maxima is integrate.

@fchapoton
Copy link
Contributor Author

comment:5

The change to TESTS:: was just something I could not keep like that once I had seen it.

I am not sure that the change to integration doctest is ok. If it fails on your machine, then you can set this ticket back to needs work.

@mforets
Copy link
Mannequin

mforets mannequin commented Apr 20, 2017

comment:6

failing in my machine with:

Doctesting 1 file.
sage -t src/sage/calculus/calculus.py
**********************************************************************
File "src/sage/calculus/calculus.py", line 1365, in sage.calculus.calculus.laplace
Failed example:
    (p1+p2).save(os.path.join(SAGE_TMP, "de_plot.png"))
Expected nothing
Got:
    doctest:warning
      File "/Users/forets/sage-src/sage/src/bin/sage-runtests", line 89, in <module>
        err = DC.run()
      File "/Users/forets/sage-src/sage/local/lib/python2.7/site-packages/sage/doctest/control.py", line 1134, in run
        self.run_doctests()
      File "/Users/forets/sage-src/sage/local/lib/python2.7/site-packages/sage/doctest/control.py", line 858, in run_doctests
        self.dispatcher.dispatch()
      File "/Users/forets/sage-src/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 1705, in dispatch
        self.parallel_dispatch()
      File "/Users/forets/sage-src/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 1595, in parallel_dispatch
        w.start()  # This might take some time
      File "/Users/forets/sage-src/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 1871, in start
        super(DocTestWorker, self).start()
      File "/Users/forets/sage-src/sage/local/lib/python2.7/multiprocessing/process.py", line 130, in start
        self._popen = Popen(self)
      File "/Users/forets/sage-src/sage/local/lib/python2.7/multiprocessing/forking.py", line 126, in __init__
        code = process_obj._bootstrap()
      File "/Users/forets/sage-src/sage/local/lib/python2.7/multiprocessing/process.py", line 258, in _bootstrap
        self.run()
      File "/Users/forets/sage-src/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 1844, in run
        task(self.options, self.outtmpfile, msgpipe, self.result_queue)
      File "/Users/forets/sage-src/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 2137, in __call__
        runner.run(test)
      File "/Users/forets/sage-src/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 641, in run
        return self._run(test, compileflags, out)
      File "/Users/forets/sage-src/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 503, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/Users/forets/sage-src/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 866, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.calculus.calculus.laplace[23]>", line 1, in <module>
        (p1+p2).save(os.path.join(SAGE_TMP, "de_plot.png"))
      File "/Users/forets/sage-src/sage/local/lib/python2.7/site-packages/sage/misc/decorators.py", line 471, in wrapper
        return func(*args, **kwds)
      File "/Users/forets/sage-src/sage/local/lib/python2.7/site-packages/sage/plot/graphics.py", line 3170, in save
        figure = self.matplotlib(**options)
      File "/Users/forets/sage-src/sage/local/lib/python2.7/site-packages/sage/plot/graphics.py", line 2566, in matplotlib
        from matplotlib.figure import Figure
      File "/Users/forets/sage-src/sage/local/lib/python2.7/site-packages/matplotlib/figure.py", line 38, in <module>
        import matplotlib.colorbar as cbar
      File "/Users/forets/sage-src/sage/local/lib/python2.7/site-packages/matplotlib/colorbar.py", line 34, in <module>
        import matplotlib.collections as collections
      File "/Users/forets/sage-src/sage/local/lib/python2.7/site-packages/matplotlib/collections.py", line 27, in <module>
        import matplotlib.backend_bases as backend_bases
      File "/Users/forets/sage-src/sage/local/lib/python2.7/site-packages/matplotlib/backend_bases.py", line 62, in <module>
        import matplotlib.textpath as textpath
      File "/Users/forets/sage-src/sage/local/lib/python2.7/site-packages/matplotlib/textpath.py", line 15, in <module>
        import matplotlib.font_manager as font_manager
      File "/Users/forets/sage-src/sage/local/lib/python2.7/site-packages/matplotlib/font_manager.py", line 1413, in <module>
        fontManager = pickle_load(_fmcache)
      File "/Users/forets/sage-src/sage/local/lib/python2.7/site-packages/matplotlib/font_manager.py", line 965, in pickle_load
        data = pickle.load(fh)
      File "/Users/forets/sage-src/sage/local/lib/python2.7/site-packages/matplotlib/font_manager.py", line 1044, in __init__
        self.ttffiles = findSystemFonts(paths) + findSystemFonts()
      File "/Users/forets/sage-src/sage/local/lib/python2.7/site-packages/matplotlib/font_manager.py", line 324, in findSystemFonts
        for f in get_fontconfig_fonts(fontext):
      File "/Users/forets/sage-src/sage/local/lib/python2.7/site-packages/matplotlib/font_manager.py", line 273, in get_fontconfig_fonts
        warnings.warn('Matplotlib is building the font cache using fc-list. This may take a moment.')
    :
    UserWarning: Matplotlib is building the font cache using fc-list. This may take a moment.
**********************************************************************
File "src/sage/calculus/calculus.py", line 1407, in sage.calculus.calculus.laplace
Failed example:
    laplace(t^n, t, s, algorithm='giac')
Exception raised:
    Traceback (most recent call last):
      File "/Users/forets/sage-src/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 503, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/Users/forets/sage-src/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 866, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.calculus.calculus.laplace[34]>", line 1, in <module>
        laplace(t**n, t, s, algorithm='giac')
      File "/Users/forets/sage-src/sage/local/lib/python2.7/site-packages/sage/calculus/calculus.py", line 1456, in laplace
        return result.sage()
      File "/Users/forets/sage-src/sage/local/lib/python2.7/site-packages/sage/interfaces/interface.py", line 1053, in sage
        return self._sage_(*args, **kwds)
      File "/Users/forets/sage-src/sage/local/lib/python2.7/site-packages/sage/interfaces/giac.py", line 1095, in _sage_
        raise NotImplementedError("Unable to parse Giac output: %s" % result)
    NotImplementedError: Unable to parse Giac output: integrate(t^n*exp(-s*t),t,0,+infinity)
**********************************************************************
1 item had failures:
   2 of  39 in sage.calculus.calculus.laplace
    [419 tests, 2 failures, 11.53 s]
----------------------------------------------------------------------
sage -t src/sage/calculus/calculus.py  # 2 doctests failed
----------------------------------------------------------------------
Total time for all tests: 11.6 seconds
    cpu time: 10.8 seconds
    cumulative wall time: 11.5 seconds

@mforets mforets mannequin added s: needs work and removed s: positive review labels Apr 20, 2017
@tscrim
Copy link
Collaborator

tscrim commented Apr 20, 2017

comment:7

The first failure is not a true failure and is independent. We need to figure out what makes this test pass on some systems and not on others.

@fchapoton

This comment has been minimized.

@fchapoton fchapoton changed the title fix a calculus doctest fix a calculus doctest (giac, laplace, integration) May 4, 2017
@fchapoton
Copy link
Contributor Author

comment:9

I see this failure with giac 1.2.3.

The branch works for me.

This problem is rather annoying, it prevents my patchbot from giving any green light..

@fchapoton
Copy link
Contributor Author

comment:10

@mforets, could you try the following, please:

sage: var('t,n,s')
sage: ex=t^n
sage: from sage.interfaces.giac import giac
sage: giac.laplace(ex, t, s)
integration(t^n*exp(-s*t),t,0,+infinity)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 4, 2017

Branch pushed to git repo; I updated commit sha1. New commits:

d79c73fMerge branch 'u/chapoton/22833' in 8.0.b4
200ba71trac 22833 adding a conversion back from giac for definite integrals

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 4, 2017

Changed commit from 2186aca to 200ba71

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 4, 2017

Branch pushed to git repo; I updated commit sha1. New commits:

79442fetrac 222833 detail

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 4, 2017

Changed commit from 200ba71 to 79442fe

@fchapoton
Copy link
Contributor Author

comment:13

And also tell me what you get when doing

sage: %giac

giac: Int(exp(x),x,0,1)
integration(exp(x),x,0,1)

@mforets
Copy link
Mannequin

mforets mannequin commented May 4, 2017

comment:14

Replying to @fchapoton:

And also tell me what you get when doing

sage: %giac

giac: Int(exp(x),x,0,1)
integration(exp(x),x,0,1)

EDIT: no no, ignore my last message, I hadn't notice that you pushed changes < 1hr ago.

i've recompiled this branch and i'm getting:

sage: %giac


  --> Switching to Giac <--

giac: Int(exp(x),x,0,1)
integrate(exp(x),x,0,1)

@mforets
Copy link
Mannequin

mforets mannequin commented May 4, 2017

comment:15

in my machine, and for the commit 79442f3, i have

sage: var('t,n,s')
(t, n, s)
sage: ex=t^n
sage: from sage.interfaces.giac import giac
sage: giac.laplace(ex, t, s)
integrate(t^n*exp(-s*t),t,0,+infinity)
sage: %giac
 
  --> Switching to Giac <--
 
giac: Int(exp(x),x,0,1)

integrate(exp(x),x,0,1)

and i still get:

sage: laplace(t^n*exp(-s*t),t,s,algorithm='giac')
...
NotImplementedError: Unable to parse Giac output: integrate(t^n*exp(-2*s*t),t,0,+infinity)

this exception is raised when we transform back to sage. maybe it's better to mimick the unevaluated laplace as with the SymPy case, that is:

...
if 'integrate' in format(result):
   return dummy_laplace(ex, t, s)
else:
   return result.sage()

i git pulled and re-compiled everything, so i ignore why you get "integration" instead.

that said, if you have pressing need then you may as well consider removing this "no-go" test altogether..

@mforets
Copy link
Mannequin

mforets mannequin commented May 4, 2017

comment:16

Replying to @mforets:

...
if 'integrate' in format(result):
   return dummy_laplace(ex, t, s)
else:
   return result.sage()

with that modif, the expected output for the test at hand becomes: laplace(t^n, t, s)

in my machine the updated code passes. let me know if you want me to push that or try something else.

@tscrim
Copy link
Collaborator

tscrim commented May 11, 2017

comment:29

Also, do you have

sage: symbol_table['giac']
{'(1+sqrt(5))/2': golden_ratio,
 'Dirac': dirac_delta,
 'Heaviside': heaviside,
 'pi': pi}

@fchapoton
Copy link
Contributor Author

comment:30
'integrate': integrate,
 'integration': integration,

in symbol_table['functions']

and

sage: symbol_table['giac']

{'(1+sqrt(5))/2': golden_ratio,
 'Dirac': dirac_delta,
 'Heaviside': heaviside,
 'pi': pi}

@tscrim
Copy link
Collaborator

tscrim commented May 11, 2017

comment:31

That's the difference: I don't have the 'integration' in the functions symbol table. What is the integration function, specifically, what does import_statements give?

@fchapoton
Copy link
Contributor Author

comment:32
sage: import_statements('integration')
# **Warning**: distinct objects with name 'integration' in:
#   - sage.calculus
#   - sage.symbolic
import sage.symbolic.integration

@tscrim
Copy link
Collaborator

tscrim commented May 11, 2017

comment:33

Sorry, I was vague, I meant:

sage: import_statements(symbol_table['functions]['integration'])

@fchapoton
Copy link
Contributor Author

comment:34
sage: symbol_table['functions']['integration']
integration
sage: import_statements(_)
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)

ValueError: no import statement found for 'integration'.

@fchapoton
Copy link
Contributor Author

comment:35
sage: symbol_table['functions']['integration']
integration
sage: type(_)
<class 'sage.symbolic.function_factory.NewSymbolicFunction'>

@tscrim
Copy link
Collaborator

tscrim commented May 11, 2017

comment:36

Now I'm not so sure about what to look for or ask, but I'm investigating. Ralf, do you have any ideas?

@tscrim
Copy link
Collaborator

tscrim commented May 11, 2017

comment:37

So I think what is happening is that integration is being considered by Maxima as a formal function. Hence, it is not simplified as an integral, and I don't think you could manipulate it as a function.

@mforets
Copy link
Mannequin

mforets mannequin commented May 11, 2017

comment:38

does this pass?

...
if 'integrate' in format(result) or 'integration' in format(result):
   return dummy_laplace(ex, t, s)
else:
   return result.sage()

@rwst
Copy link

rwst commented May 12, 2017

comment:39

Let me try to summarize. We cannot fix the doctest because of conflicting results due to giac's clever use of locale which breaks its interface to the world. The immediate way to adapt to this IMO is to look for both integrate and integration in the result when using laplace of giac. The more far-sighted way would be to fix the giac-Sage interface so that in the laplace code we can just say return result.sage() unconditionally.

@fchapoton
Copy link
Contributor Author

comment:40

could somebody please take action here, and propose a better branch ?

@strogdon
Copy link

comment:41

What is the desired output of

sage: laplace(t^n, t, s, algorithm='giac')

If it is integration(t<sup>n*e</sup>(-s*t), t, 0, +Infinity) then one could add

integrate integration

to local/share/giac/doc/en/keywords and any other provided $LANG/keywords file. Doing this I get

sage: var('t,n,s')
(t, n, s)
sage: ex=t^n
sage: from sage.interfaces.giac import giac
sage: giac.laplace(ex, t, s)
integration(t^n*exp(-s*t),t,0,+infinity)
sage: %giac

  --> Switching to Giac <--

giac: Int(exp(x),x,0,1)
integration(exp(x),x,0,1)

Without the change the doctest would pass but with the change I get

sage -t --long src/sage/calculus/calculus.py
**********************************************************************
File "src/sage/calculus/calculus.py", line 1406, in sage.calculus.calculus.laplace
Failed example:
    laplace(t^n, t, s, algorithm='giac')
Expected:
    Traceback (most recent call last):
    ...
    NotImplementedError: Unable to parse Giac output: integrate(t^n*exp(-s*t),t,0,+infinity)
Got:
    integration(t^n*e^(-s*t), t, 0, +Infinity)

@mforets
Copy link
Mannequin

mforets mannequin commented May 13, 2017

comment:42

again, for consistency the desired output of laplace(t^n, t, s, algorithm='giac') should be the same as the other two interfaces when they don't know the answer, don't you think?

from your answers, i can reproduce the issue playing with $ export LANG=fr_FR.UTF-8 and $ export LANG=en_US.UTF-8 in the shell :)

hence, let me upload a branch doing rws's comment:39 and my comment:38, if i understood correctly.

@mforets
Copy link
Mannequin

mforets mannequin commented May 13, 2017

New commits:

e2d74c6parse unevaluated expression in EN and FR

@mforets
Copy link
Mannequin

mforets mannequin commented May 13, 2017

Changed commit from 79442fe to e2d74c6

@mforets
Copy link
Mannequin

mforets mannequin commented May 13, 2017

Changed author from Frédéric Chapoton to Frédéric Chapoton, Marcelo Forets

@mforets
Copy link
Mannequin

mforets mannequin commented May 13, 2017

Changed branch from u/chapoton/22833 to u/mforets/22833

@mforets mforets mannequin added s: needs review and removed s: needs work labels May 13, 2017
@egourgoulhon
Copy link
Member

comment:44

Just to confirm that merging this branch in Sage 8.0.beta6 fixes the doctest error on my system.

@strogdon
Copy link

Changed reviewer from Travis Scrimshaw to Travis Scrimshaw, Steven Trogdon

@strogdon
Copy link

comment:45

I think this is good.

@mforets
Copy link
Mannequin

mforets mannequin commented May 14, 2017

comment:46

Thanks for the review.

Here are two somehow related issues: #22997 and #22995.

@vbraun
Copy link
Member

vbraun commented May 15, 2017

Changed branch from u/mforets/22833 to e2d74c6

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

6 participants