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

Introduce graphics insets #27866

Closed
egourgoulhon opened this issue May 24, 2019 · 37 comments
Closed

Introduce graphics insets #27866

egourgoulhon opened this issue May 24, 2019 · 37 comments

Comments

@egourgoulhon
Copy link
Member

This ticket introduces graphics insets, i.e. it allows to have various Graphics objects at arbitrary locations and sizes on a single figure. Previously, only graphics arrays, i.e. Graphics objects arranged in a regular array, were possible.

This is made possible thanks to the generic class MultiGraphics introduced in #27865.
The user interface is constituted of three new functions:

  • the method Graphics.inset(), which adds an inset to a given graphics; see this preview example
  • the method MultiGraphics.inset(), which adds an inset to a multi-graphics (e.g. a GraphicsArray); see this preview example
  • the function multi_graphics() , which creates a figure from a list of Graphics objects at user-specified locations; see this preview example

Depends on #27865

CC: @kcrisman @fchapoton @dkrenn @jdemeyer @embray

Component: graphics

Keywords: graphics inset

Author: Eric Gourgoulhon

Branch: ed586d3

Reviewer: Markus Wageringel

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

@egourgoulhon egourgoulhon added this to the sage-8.8 milestone May 24, 2019
@egourgoulhon
Copy link
Member Author

Author: Eric Gourgoulhon

@egourgoulhon
Copy link
Member Author

Branch: public/graphics/graphics_insets

@egourgoulhon
Copy link
Member Author

Commit: e177646

@egourgoulhon
Copy link
Member Author

Dependencies: #27865

@egourgoulhon
Copy link
Member Author

Last 10 new commits:

1bf8ad1Small cleaning in MultiGraphics
ccff60cFix some doctests after the improved computation of nrows and ncols in graphics_array
26180abSome cleaning in MultiGraphics.matplotlib
3bd6a56Minor cleaning in GraphicsArray
3a5ef55Change index range in GraphicsArray._add_subplot()
60ede56First draft of multi_graphics
e58adf3Add method inset() to Graphics and MultiGraphics
26a8eb5Add method position in MultiGraphics and GraphicsArray
8ce0db3Add transparency of individual graphics in MultiGraphics + more doctests
e177646Add documentation to MultiGraphics

@egourgoulhon

This comment has been minimized.

@embray
Copy link
Contributor

embray commented Jul 3, 2019

comment:5

Moving tickets from the Sage 8.8 milestone that have been actively worked on in the last six months to the next release milestone (optimistically).

@embray embray modified the milestones: sage-8.8, sage-8.9 Jul 3, 2019
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 6, 2019

Changed commit from e177646 to f504d7a

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 6, 2019

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

cfb4c35Merge branch 'public/graphics/graphics_insets' of git://trac.sagemath.org/sage into Sage 8.9.beta5
f504d7aCorrect a typo in plot.py

@egourgoulhon
Copy link
Member Author

comment:7

The two pyflakes errors reported by the patchbot are spurious ones: these imports are really necessary.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 25, 2019

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

3099f4827866: fix typos

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 25, 2019

Changed commit from f504d7a to 3099f48

@mwageringel
Copy link

comment:9

I was wondering what happens if the graphics inset requires a particular aspect ratio, and apparently the aspect ratio is preserved, which is good. However, in the cases I tested I obtained an inset that is much larger than expected. Could you explain the output for g2 and g3?

sage: g1 = plot(x^3, x, -1, 1, aspect_ratio=1)
sage: g2 = g1.inset(circle((0,1000), 1, frame=True, aspect_ratio=1), (.5,.5,.5,.5))
sage: g3 = g1.inset(circle((0,1000), 1, frame=True, aspect_ratio='automatic'), (.5,.5,.5,.5))

Edit: To be more precise, the inset has the correct size relative to the final image, but not in relation to the main graphics.

@egourgoulhon
Copy link
Member Author

comment:10

Attachment: g2.png

Replying to @mwageringel:

Edit: To be more precise, the inset has the correct size relative to the final image, but not in relation to the main graphics.

Actually, this is a feature, not a bug. From the documentation returned by g1.inset?:

Signature:      g1.inset(graphics, pos=None, fontsize=None)
Docstring:     
   Add a graphics object as an inset.

   INPUT:

   * "graphics" -- the graphics object (instance of "Graphics") to
     be added as an inset to the current graphics

   * "pos" -- (default: "None") 4-tuple "(left, bottom, width,
     height)" specifying the location and size of the inset on the
     figure, all quantities being in fractions of the figure width and
     height; if "None", the value "(0.70, 0.68, 0.2, 0.2)" is used

   * "fontsize" -- (default: "None")  integer, font size (in points)
     for the inset; if "None", the value of 6 points is used, unless
     "fontsize" has been explicitely set in the construction of
     "graphics" (in this case, it is not overwritten here)

   OUTPUT:

   * instance of "MultiGraphics"

As you can see, the argument pos specifies the position and size of the inset relative to the figure size. So, with pos=(0.5, 0.5, 0.5, 0.5), as in your example g2, one obtains an inset that has half the figure width and half the figure height. Do you think that the documentation should be improved about this?

@mwageringel
Copy link

comment:11

Should it be half the width and height of the figure g1 or the figure g2? It seems to be half the size of g2, but I would have expected it to be half the size of g1 as well. In other words, g1 appears too small in g2 – I would have expected g1 to fill out the figure g2.

As far as I understand, the position is computed in terms of the figure size, not the plotting area, so I should not expect the inset to cover the top right quadrant of the plot precisely, but in g2 the positioning of the inset seems a bit too far off. In particular, I would like to understand whether the tick labels count towards the width and height given in pos or whether these parameters just determine the size of the frame of the inset. From the looks of it, horizontal labels do not count towards the height, while vertical ones seem to add to the width?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 28, 2019

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

3d68cedMerge branch 'public/graphics/graphics_insets' of git://trac.sagemath.org/sage into Sage 8.9.beta8
71dc5c8Improve size adjustment in graphics insets

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 28, 2019

Changed commit from 3099f48 to 71dc5c8

@egourgoulhon
Copy link
Member Author

Attachment: g2_new.png

@egourgoulhon
Copy link
Member Author

comment:13

Replying to @mwageringel:

Should it be half the width and height of the figure g1 or the figure g2? It seems to be half the size of g2, but I would have expected it to be half the size of g1 as well. In other words, g1 appears too small in g2 – I would have expected g1 to fill out the figure g2.

I've changed the rescaling of the base plot when generating the MultiGraphics object, the previous values were indeed shrinking g1 too much. The new output is in the attached figure g2_new.png.

As far as I understand, the position is computed in terms of the figure size, not the plotting area, so I should not expect the inset to cover the top right quadrant of the plot precisely, but in g2 the positioning of the inset seems a bit too far off. In particular, I would like to understand whether the tick labels count towards the width and height given in pos or whether these parameters just determine the size of the frame of the inset. From the looks of it, horizontal labels do not count towards the height, while vertical ones seem to add to the width?

The position/size given in the argument pos regard the position/size of the frame box of the inset with respect to the final plotting area (canvas), not with respect to g1. This is a feature we should keep IMHO, since it matches with Matplotlib conventions, cf. the argument rect in the method Figure.add_axes at https://matplotlib.org/api/_as_gen/matplotlib.figure.Figure.html.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 29, 2019

Changed commit from 71dc5c8 to d34b69f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 29, 2019

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

d34b69fRestore file permissions on build/pkgs/flint/spkg-check

@egourgoulhon
Copy link
Member Author

comment:15

For some reason, yesterday's commit (comment:12) contained a spurious change of permissions on the file build/pkgs/flint/spkg-check. The above commit (comment:14) corrects this.

@mwageringel
Copy link

comment:16

Attachment: g4.png

There still seems to be an inconsistency between vertical and horizontal positioning. For example, in g4 the blue squares touch each other in one direction, but not the other. I am not sure what the preferred behaviour is in this case, but it should be the same in both directions.

g1 = plot(x^3, x, -1, 1, aspect_ratio=1, color='red', frame=True)
g1.axes_color('red')
c = circle((0,1000), 1, frame=True, aspect_ratio=1, transparent=True)
c.axes_color('blue')
g4 = g1.inset(c, (0,0,.5,.5)).inset(c, (0,.5,.5,.5)).inset(c, (.5,0,.5,.5)).inset(c, (.5,.5,.5,.5))


Moreover, some of the type checks testing for lists and tuples seem a bit restrictive. Especially this one is not necessary, as the implementation will work for any iterable and iterator, so the type check should just be removed:

        if not isinstance(graphics_list, (list, tuple)):
            raise TypeError("graphics_list must be a list or a tuple, "
                            "not {}".format(graphics_list))

I am not sure why the following change is needed. Would it be better to test for the converse, i.e. if not isinstance... of the types you want to exclude? Or are Graphics and GraphicsArray the only types that support tight_layout? Also, there is a similar change in multigraphics.py that only tests for isinstance(self, GraphicsArray). If you think it is preferable to keep this as is, that would also be fine be me.

-            # tight_layout adjusts the *subplot* parameters so ticks aren't
-            # cut off, etc.
-            figure.tight_layout()
+            if isinstance(graphics, (sage.plot.graphics.Graphics,
+                                     sage.plot.multigraphics.GraphicsArray)):
+                # for Graphics and GraphicsArray, tight_layout adjusts the
+                # *subplot* parameters so ticks aren't cut off, etc.
+                figure.tight_layout()

Other than that, everything looks good to me.

@mwageringel
Copy link

Reviewer: Markus Wageringel

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 24, 2019

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

2600bd7Fix merge conflict of graphics insets branch with Sage 9.0.beta2

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 24, 2019

Changed commit from d34b69f to 2600bd7

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 25, 2019

Changed commit from 2600bd7 to ed586d3

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 25, 2019

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

ed586d3Remove restrictive type check in MultiGraphics

@egourgoulhon
Copy link
Member Author

Attachment: g4_aspect_ratio1.png

@egourgoulhon
Copy link
Member Author

comment:19

Sorry for the delay in replying...

Replying to @mwageringel:

There still seems to be an inconsistency between vertical and horizontal positioning. For example, in g4 the blue squares touch each other in one direction, but not the other. I am not sure what the preferred behaviour is in this case, but it should be the same in both directions.

Actually this is not a bug but results from a clash between the default aspect ratio of the whole figure, which is 4/3, and the aspect ratio 1 of the subplots. If you ask for an aspect ratio of 1 for the whole figure, then everything is OK:

show(g4, figsize=(5, 5))

leads to

Moreover, some of the type checks testing for lists and tuples seem a bit restrictive. Especially this one is not necessary, as the implementation will work for any iterable and iterator, so the type check should just be removed:

Indeed! Thanks for pointing this. The type check has been removed in the above commit.

I am not sure why the following change is needed. Would it be better to test for the converse, i.e. if not isinstance... of the types you want to exclude? Or are Graphics and GraphicsArray the only types that support tight_layout?

Yes, I think so.

Also, there is a similar change in multigraphics.py that only tests for isinstance(self, GraphicsArray). If you think it is preferable to keep this as is, that would also be fine be me.

Yes I think it is preferable to keep the tests as they are, since only GraphicsArray requires the tight layout.

@mwageringel
Copy link

comment:21

Ok, thanks for the explanations. I am setting this to positive then.

@egourgoulhon
Copy link
Member Author

comment:23

Replying to @mwageringel:

Ok, thanks for the explanations. I am setting this to positive then.

Thanks for the review!

@vbraun
Copy link
Member

vbraun commented Oct 27, 2019

Changed branch from public/graphics/graphics_insets to ed586d3

@antonio-rojas
Copy link
Contributor

comment:25

This is causing two test failures on Arch when compiling against system libraries (haven't tried on sage-the-distro)

**********************************************************************
File "/usr/lib/python3.7/site-packages/sage/plot/multigraphics.py", line 1297, in sage.plot.multigraphics.GraphicsArray.position
Failed example:
    G.position(0)  # tol 1.0e-13
Expected:
    (0.028906249999999998,
     0.038541666666666696,
     0.45390624999999996,
     0.9229166666666667)
Got:
    (0.023437500000000003,
     0.03415488992713045,
     0.4627803348994754,
     0.9345951100728696)
Tolerance exceeded in 4 of 4:
    0.028906249999999998 vs 0.023437500000000003, tolerance 2e-1 > 1e-13
    0.038541666666666696 vs 0.03415488992713045, tolerance 2e-1 > 1e-13
    0.45390624999999996 vs 0.4627803348994754, tolerance 2e-2 > 1e-13
    0.9229166666666667 vs 0.9345951100728696, tolerance 2e-2 > 1e-13
**********************************************************************
File "/usr/lib/python3.7/site-packages/sage/plot/multigraphics.py", line 1302, in sage.plot.multigraphics.GraphicsArray.position
Failed example:
    G.position(1)  # tol 1.0e-13
Expected:
    (0.5171874999999999,
     0.038541666666666696,
     0.45390624999999996,
     0.9229166666666667)
Got:
    (0.5136230468749999,
     0.19293222169724827,
     0.46278033489947534,
     0.617040446532634)
Tolerance exceeded in 4 of 4:
    0.5171874999999999 vs 0.5136230468749999, tolerance 7e-3 > 1e-13
    0.038541666666666696 vs 0.19293222169724827, tolerance 5e0 > 1e-13
    0.45390624999999996 vs 0.46278033489947534, tolerance 2e-2 > 1e-13
    0.9229166666666667 vs 0.617040446532634, tolerance 4e-1 > 1e-13
**********************************************************************
1 item had failures:
   2 of   6 in sage.plot.multigraphics.GraphicsArray.position
    [192 tests, 2 failures, 15.63 s]

@antonio-rojas
Copy link
Contributor

Changed commit from ed586d3 to none

@vbraun
Copy link
Member

vbraun commented Nov 3, 2019

comment:26

New ticket please!

@jhpalmieri
Copy link
Member

comment:27

See #29547.

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