Opened 6 years ago

Closed 6 years ago

#18637 closed enhancement (fixed)

Fix some optional/not tested tags

Reported by: jdemeyer Owned by:
Priority: minor Milestone: sage-6.8
Component: doctest coverage Keywords:
Cc: Merged in:
Authors: Jeroen Demeyer Reviewers: John Palmieri
Report Upstream: N/A Work issues:
Branch: 8036f11 (Commits) Commit: 8036f11f215a1ae39bab8c89b0d818481b3446a6
Dependencies: Stopgaps:

Description

Fix all # optional tags without a package name (except for giac which is a different ticket) and replace some # not tested tags by # known bug.

Change History (15)

comment:1 Changed 6 years ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/18637

comment:2 Changed 6 years ago by git

  • Commit set to 23736ea85fa50142a3e1fd3fe7ca4c0ce34669f5

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

23736eaFix some optional/not tested tags

comment:3 Changed 6 years ago by jdemeyer

  • Status changed from new to needs_review

comment:4 Changed 6 years ago by jhpalmieri

A few tests marked ImageMagick fail. Should we fix them here or elsewhere? These changes work for me:

  • src/sage/misc/latex.py

    diff --git a/src/sage/misc/latex.py b/src/sage/misc/latex.py
    index ef76d09..6b47e20 100644
    a b class Latex(LatexCall): 
    10841084            An error
    10851085            ...
    10861086            No pages of output.
    1087             <BLANKLINE>
     1087            ...
    10881088        """
    10891089        MACROS = latex_extra_preamble()
    10901090
  • src/sage/plot/animate.py

    diff --git a/src/sage/plot/animate.py b/src/sage/plot/animate.py
    index 83cb499..005dcde 100644
    a b import struct 
    118118import zlib
    119119
    120120from sage.structure.sage_object import SageObject
    121 from sage.misc.temporary_file import tmp_dir, tmp_filename, graphics_filename
     121from sage.misc.temporary_file import tmp_dir, tmp_filename
    122122import plot
    123123import sage.misc.misc
    124124import sage.misc.viewer
    www.ffmpeg.org, or use 'convert' to produce gifs instead.""" 
    601601                raise OSError(msg)
    602602        else:
    603603            if not savefile:
    604                 savefile = graphics_filename(ext='.gif')
     604                savefile = tmp_filename(ext='.gif')
    605605            if not savefile.endswith('.gif'):
    606606                savefile += '.gif'
    607607            savefile = os.path.abspath(savefile)
    please install it and try again.""" 
    812812                else:
    813813                    if output_format[0] != '.':
    814814                        output_format = '.'+output_format
    815                 savefile = graphics_filename(ext=output_format)
     815                savefile = tmp_filename(ext=output_format)
    816816            else:
    817817                if output_format is None:
    818818                    suffix = os.path.splitext(savefile)[1]
    please install it and try again.""" 
    917917        """
    918918        pngdir = self.png()
    919919        if savefile is None:
    920             savefile = graphics_filename('.png')
     920            savefile = tmp_filename('.png')
    921921        with open(savefile, "wb") as out:
    922922            apng = APngAssembler(
    923923                out, len(self),

comment:5 Changed 6 years ago by jhpalmieri

Sorry, I missed this change:

  • src/sage/plot/animate.py

    diff --git a/src/sage/plot/animate.py b/src/sage/plot/animate.py
    index 83cb499..a6bf569 100644
    a b Animate using FFmpeg_ instead of ImageMagick:: 
    4343Animate as an APNG_::
    4444
    4545    sage: a.apng()  # long time
    46     doctest:...: DeprecationWarning: use tmp_filename instead
    47     See http://trac.sagemath.org/17234 for details.
    4846
    4947An animated :class:`sage.plot.graphics.GraphicsArray` of rotating ellipses::
    5048

comment:6 follow-up: Changed 6 years ago by jdemeyer

I prefer to move the changes graphics_filename -> tmp_filename to a different ticket, since that's not just about doctests, but about code.

comment:7 Changed 6 years ago by jhpalmieri

Here are some other tests which could be fixed (they include extra commas):

databases/oeis.py:1640:            sage: f = oeis(45) ; f                      # optional -- internet, webbrowser
databases/oeis.py:1643:            sage: f.browse()                            # optional -- internet, webbrowser
rings/number_field/number_field.py:4738:            sage: NumberField(x^3 + 2*x + 1, 'a').galois_group(algorithm='magma')   # optional - magma, , database_gap
rings/polynomial/polynomial_rational_flint.pyx:1553:            sage: f.galois_group(algorithm='magma')  # optional - magma, database_gap

comment:8 Changed 6 years ago by git

  • Commit changed from 23736ea85fa50142a3e1fd3fe7ca4c0ce34669f5 to d63b0c9834909b81dbf44eae2a64e60ab9b3189c

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

d63b0c9Minor optional doctest fixes

comment:9 in reply to: ↑ 6 ; follow-up: Changed 6 years ago by jhpalmieri

Replying to jdemeyer:

I prefer to move the changes graphics_filename -> tmp_filename to a different ticket, since that's not just about doctests, but about code.

I agree, especially since on further investigation, my changes don't work. Doctests pass, but the commands no longer behave correctly in the notebook.

Can we make the change to latex.py here? That's a very minor doctest fix.

comment:10 in reply to: ↑ 9 ; follow-up: Changed 6 years ago by jdemeyer

Replying to jhpalmieri:

Can we make the change to latex.py here? That's a very minor doctest fix.

I don't have imagemagick installed and I don't want to install it for just this ticket. What's your output of ./sage -t --optional=all src/sage/misc/latex.py with the current branch (but without your changes)?

comment:11 in reply to: ↑ 10 Changed 6 years ago by jhpalmieri

Replying to jdemeyer:

Replying to jhpalmieri:

Can we make the change to latex.py here? That's a very minor doctest fix.

I don't have imagemagick installed and I don't want to install it for just this ticket. What's your output of ./sage -t --optional=all src/sage/misc/latex.py with the current branch (but without your changes)?

There one long failed doctest, which I will abbreviate:

File "src/sage/misc/latex.py", line 1083, in sage.misc.latex.Latex.?
Failed example:
    latex.eval("\ThisIsAnInvalidCommand", {}) # optional -- ImageMagick
Expected:
    An error
    ...
    No pages of output.
    <BLANKLINE>
Got:
    An error occurred.
    This is pdfTeX, Version 3.14159265-2.6-1.40.15 (TeX Live 2014) (preloaded format=pdflatex 2014.9.22)  8 JUN 2015 12:34
    entering extended mode
     restricted \write18 enabled.
     %&-line parsing enabled.

        [snip]

    <BLANKLINE>
    (./sage43.aux) ) 
    Here is how much of TeX's memory you used:
     2758 strings out of 493108
     34433 string characters out of 6134847
     90768 words of memory out of 5000000
     6200 multiletter control sequences out of 15000+600000
     4403 words of font info for 15 fonts, out of 8000000 for 9000
     1141 hyphenation exceptions out of 8191
     38i,1n,26p,260b,38s stack positions out of 5000i,500n,10000p,200000b,80000s
    <BLANKLINE>
    No pages of output.
    PDF statistics:
     0 PDF objects out of 1000 (max. 8388607)
     0 named destinations out of 1000 (max. 500000)
     1 words of extra memory for PDF output out of 10000 (max. 10000000)
    <BLANKLINE>
    <BLANKLINE>
**********************************************************************
1 item had failures:
   1 of   2 in sage.misc.latex.Latex.?
    [308 tests, 1 failure, 12.37 s]
----------------------------------------------------------------------
sage -t src/sage/misc/latex.py  # 1 doctest failed
----------------------------------------------------------------------

So the issue is the extra lines starting "PDF statistics", plus two <BLANKLINE>'s at the end instead of one.

comment:12 Changed 6 years ago by git

  • Commit changed from d63b0c9834909b81dbf44eae2a64e60ab9b3189c to 8036f11f215a1ae39bab8c89b0d818481b3446a6

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

8036f11Fix doctest

comment:13 Changed 6 years ago by jdemeyer

Does this work?

comment:14 Changed 6 years ago by jhpalmieri

  • Reviewers set to John Palmieri
  • Status changed from needs_review to positive_review

Yes, thank you. Everything else looks good, and I've checked some of them by installing the appropriate optional packages. Most of the changes are obviously the right things to do, so if they break some optional doctests, the tests were broken before. I don't have magma, maple, or mathematica installed, among others, so I have not tested everything, and any resulting broken optional doctests should probably be dealt with elsewhere.

comment:15 Changed 6 years ago by vbraun

  • Branch changed from u/jdemeyer/ticket/18637 to 8036f11f215a1ae39bab8c89b0d818481b3446a6
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.