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

Method tikz of polyhedron class can now return an object of type TikzPicture #33002

Closed
seblabbe opened this issue Dec 9, 2021 · 34 comments
Closed

Comments

@seblabbe
Copy link
Contributor

seblabbe commented Dec 9, 2021

As a follow up of #20343, the current branch now allows to specify the output type of the tikz method of a polyhedron:

sage: co = polytopes.cuboctahedron()
sage: t = co.tikz([674,108,-731], 112, output_type='TikzPicture')
sage: t
\documentclass[tikz]{standalone}
\begin{document}
\begin{tikzpicture}%
        [x={(0.249656cm, -0.577639cm)},
        y={(0.777700cm, -0.358578cm)},
        z={(-0.576936cm, -0.733318cm)},
        scale=1.000000,
---
92 lines not printed (5190 characters in total).
Use print to see the full content.
---
\node[vertex] at (1.00000, 0.00000, 1.00000)     {};
\node[vertex] at (1.00000, 1.00000, 0.00000)     {};
%%
%%
\end{tikzpicture}
\end{document}
sage: t.pdf()
'/tmp/tmpobwhg3p7/tikz_o9dkz419.pdf'

We also change the default behavior in favor of output_type='TikzPicture'. A deprecation warnings is now sent when output_type is not given:

sage: co = polytopes.cuboctahedron()
sage: t = co.tikz([674,108,-731], 112)
... DeprecationWarning: Since SageMath 5.13 (ticket #12083), the method
 .tikz() of a polyhedron returns an object of type ``LatexExpr`` which is a 
Python str. Since SageMath 9.7, this default behavior of returning an object
 of type LatexExpr is deprecated as the default output will soon change to 
an object of type ``TikzPicture`` from the module sage.misc.latex_standalone
 (newly introduced in SageMath 9.6). Please update your code to specify the
 desired output type as ``.tikz(output_type='LatexExpr')`` to keep the old
 behavior or ``.tikz(output_type='TikzPicture')`` to use the future default
 behavior.
See https://trac.sagemath.org/33002 for details.

CC: @trevorkarn @jplab

Component: graphics

Author: Sébastien Labbé

Branch: 8729071

Reviewer: Laith Rastanawi

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

@seblabbe seblabbe added this to the sage-9.5 milestone Dec 9, 2021
@seblabbe
Copy link
Contributor Author

seblabbe commented Dec 9, 2021

Dependencies: #20343

@mkoeppe mkoeppe modified the milestones: sage-9.5, sage-9.6 Dec 18, 2021
@videlec
Copy link
Contributor

videlec commented Feb 24, 2022

comment:3

What would be the interaction with sage/graphs/graph_latex.py?

@mkoeppe mkoeppe modified the milestones: sage-9.6, sage-9.7 Apr 7, 2022
@seblabbe
Copy link
Contributor Author

Commit: 119baaf

@seblabbe
Copy link
Contributor Author

New commits:

119baaf33002: tikz method of polyhedron can now output a TikzPicture object

Refocussing this ticket on polyhedron only. Let's deal with tikz method for graph, posets, etc. in different tickets.

@seblabbe
Copy link
Contributor Author

Branch: u/slabbe/33002

@seblabbe
Copy link
Contributor Author

Author: Sébastien Labbé

@seblabbe

This comment has been minimized.

@seblabbe seblabbe changed the title Adding a method in graph, poset, polyhedron, etc. classes to return an object of type TikzPicture Method tikz of polyhedron class can now return an object of type TikzPicture Jun 30, 2022
@seblabbe

This comment has been minimized.

@LaisRast
Copy link

LaisRast commented Jul 5, 2022

Reviewer: Laith Rastanawi

@LaisRast
Copy link

LaisRast commented Jul 5, 2022

comment:8

I would suggest to implement this for the function Projection.tikz() (of geometry.polyhedron.plot),
and then add the parameter output_type to Polyhedron_base6.tikz().

One reason for that is sometimes you may want to modify the projection before drawing the polytope using TikZ.
For instance in #34113:
you may want to choose a different window facet for the Schlegel projection before drawing the polytope.
So something like this should work:

P.schlegel_projection(P.facets()[1]).tikz([1, 0, 0], output_type="TikzPicture")

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 6, 2022

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

33afbd333002: moving the LatexExpr vs TikzPicture output type choice inside method projection().tikz() instead of tikz()
a1d598933002: adapting the doctests with the new behavior thus also avoiding deprecation warnings

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 6, 2022

Changed commit from 119baaf to a1d5989

@seblabbe
Copy link
Contributor Author

seblabbe commented Jul 6, 2022

comment:10

Good comment. I did the change.

I also added a deprecation warning in favor of output_type='TikzPicture'.

I adapted the doctests in sage/geometry/polyhedron and in the doc.

Maybe there are remaining doctests to update? Let's see what the patchbots say.

@seblabbe
Copy link
Contributor Author

seblabbe commented Jul 6, 2022

Changed dependencies from #20343 to none

@seblabbe
Copy link
Contributor Author

seblabbe commented Jul 6, 2022

comment:11

Dear JP, it would be good for you eyes to look at the changes made in this ticket, because I am changing code and doc that you have written in the first place starting with #12083.

@seblabbe

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 21, 2022

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

7a71bbe33002: fixing small remarks in polytope_tikz.rst

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 21, 2022

Changed commit from 4ef07a9 to 7a71bbe

@seblabbe
Copy link
Contributor Author

comment:18

Replying to @LaisRast:

Seems good to me, except the following small remarks:

In polytope_tikz.rst:

  • Add full stop to the end of "... the raw TikZ picture of your polytope"
  • "you will have a file named.." -> " you will have two files.."

Thanks, i just added a commit which does that.

I suggest also:
``None``(deprecated) -> ``None`` (to be deprecated)

I think the definition of "deprecated" is that the option will be change or be gone in the future. An option is deprecated one or two years before it is gone. So, I keep it as it is for now.

Needs review. (I will be away from internet for the next two weeks).

@LaisRast
Copy link

comment:20

Looks good to me.

@vbraun
Copy link
Member

vbraun commented Jul 26, 2022

comment:21
sage -t --long --warn-long 45.5 --random-seed=123 src/sage/misc/latex_standalone.py
**********************************************************************
File "src/sage/misc/latex_standalone.py", line 161, in sage.misc.latex_standalone
Failed example:
    s = P.projection().tikz([674,108,-731],112)
Expected nothing
Got:
    doctest:warning
      File "/home/release/Sage/src/bin/sage-runtests", line 153, in <module>
        err = DC.run()
      File "/home/release/Sage/local/var/lib/sage/venv-python3.10.5/lib/python3.10/site-packages/sage/doctest/control.py", line 1376, in run
        self.run_doctests()
      File "/home/release/Sage/local/var/lib/sage/venv-python3.10.5/lib/python3.10/site-packages/sage/doctest/control.py", line 1057, in run_doctests
        self.dispatcher.dispatch()
      File "/home/release/Sage/local/var/lib/sage/venv-python3.10.5/lib/python3.10/site-packages/sage/doctest/forker.py", line 2021, in dispatch
        self.parallel_dispatch()
      File "/home/release/Sage/local/var/lib/sage/venv-python3.10.5/lib/python3.10/site-packages/sage/doctest/forker.py", line 1916, in parallel_dispatch
        w.start()  # This might take some time
      File "/home/release/Sage/local/var/lib/sage/venv-python3.10.5/lib/python3.10/site-packages/sage/doctest/forker.py", line 2190, in start
        super().start()
      File "/home/release/Sage/local/var/lib/sage/venv-python3.10.5/lib/python3.10/multiprocessing/process.py", line 121, in start
        self._popen = self._Popen(self)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.10.5/lib/python3.10/multiprocessing/context.py", line 224, in _Popen
        return _default_context.get_context().Process._Popen(process_obj)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.10.5/lib/python3.10/multiprocessing/context.py", line 277, in _Popen
        return Popen(process_obj)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.10.5/lib/python3.10/multiprocessing/popen_fork.py", line 19, in __init__
        self._launch(process_obj)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.10.5/lib/python3.10/multiprocessing/popen_fork.py", line 71, in _launch
        code = process_obj._bootstrap(parent_sentinel=child_r)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.10.5/lib/python3.10/multiprocessing/process.py", line 315, in _bootstrap
        self.run()
      File "/home/release/Sage/local/var/lib/sage/venv-python3.10.5/lib/python3.10/site-packages/sage/doctest/forker.py", line 2162, in run
        task(self.options, self.outtmpfile, msgpipe, self.result_queue)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.10.5/lib/python3.10/site-packages/sage/doctest/forker.py", line 2492, in __call__
        doctests, extras = self._run(runner, options, results)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.10.5/lib/python3.10/site-packages/sage/doctest/forker.py", line 2544, in _run
        result = runner.run(test)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.10.5/lib/python3.10/site-packages/sage/doctest/forker.py", line 866, in run
        return self._run(test, compileflags, out)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.10.5/lib/python3.10/site-packages/sage/doctest/forker.py", line 695, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.10.5/lib/python3.10/site-packages/sage/doctest/forker.py", line 1093, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.misc.latex_standalone[23]>", line 1, in <module>
        s = P.projection().tikz([Integer(674),Integer(108),-Integer(731)],Integer(112))
      File "/home/release/Sage/local/var/lib/sage/venv-python3.10.5/lib/python3.10/site-packages/sage/geometry/polyhedron/plot.py", line 1431, in tikz
        deprecation(33002, msg)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.10.5/lib/python3.10/site-packages/sage/misc/superseded.py", line 99, in deprecation
        warning(trac_number, message, DeprecationWarning, stacklevel)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.10.5/lib/python3.10/site-packages/sage/misc/superseded.py", line 180, in warning
        warn(message, warning_class, stacklevel)
      File "/home/release/Sage/local/var/lib/sage/venv-python3.10.5/lib/python3.10/warnings.py", line 109, in _showwarnmsg
        sw(msg.message, msg.category, msg.filename, msg.lineno,
    :
    DeprecationWarning: The default type of the returned object will soon be changed from `sage.misc.latex.LatexExpr` to `sage.misc.latex_standalone.TikzPicture`.  Please update your code to specify the desired output type as `.tikz(output_type='LatexExpr')` to keep the old behavior or `.tikz(output_type='TikzPicture')` to use the future default behavior.
    See https://trac.sagemath.org/33002 for details.
**********************************************************************

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 9, 2022

Changed commit from 7a71bbe to 8729071

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 9, 2022

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

872907133002: updating doctest to fix deprecation warning

@seblabbe
Copy link
Contributor Author

seblabbe commented Aug 9, 2022

comment:23

Oups, I forgot that one. The other failures seen by the patchbot (related to giac) are unrelated.

Needs review.

@seblabbe
Copy link
Contributor Author

comment:24

ping!

Maybe there is still a chance for this to get in 9.7 ...

@LaisRast
Copy link

LaisRast commented Sep 4, 2022

comment:25

Sorry for the late reply.
Since now you fixed the test, the ticket is done.

@vbraun
Copy link
Member

vbraun commented Sep 20, 2022

Changed branch from u/slabbe/33002 to 8729071

@seblabbe
Copy link
Contributor Author

comment:28

I created #34606 to fix the doc, because this ticket was merged in 9.8 not in 9.7.

@seblabbe
Copy link
Contributor Author

Changed commit from 8729071 to none

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

5 participants