Opened 12 years ago

Closed 8 years ago

#2100 closed enhancement (fixed)

sensible defaults for aspect ratio

Reported by: jason Owned by: jason
Priority: major Milestone: sage-4.7.2
Component: graphics Keywords: sd31
Cc: kcrisman, mhampton, mpatel Merged in: sage-4.7.2.alpha0
Authors: Jason Grout, Karl-Dieter Crisman Reviewers: Andrey Novoseltsev, Karl-Dieter Crisman, Ryan Grout
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by kcrisman)

We have two concepts of "aspect ratio" that we'd like to expose to the user.

This patch exposes the following behavior:

    * Figure aspect ratio -- controls the final image size, including any text labels, etc. This will be set using the following options:
          o figsize
                + single number - use this as a base size for the width of the figure canvas
                + two numbers - an actual figure size in inches
                + None - use the default size in Matplotlib
          o fit_tight
                + if True, clip or extend the resulting figure to the plot objects (so the resulting figure will probably not have figsize dimensions
                + if False, the resulting figure has exactly figsize dimensions, but items in the plot may be clipped or there may be excessive padding around drawn objects
    * Pixel aspect ratio
          o aspect_ratio -- the ratio height/width for a unit square
                + 1 -- a unit square appears to have equal height and width.  This will be the default for Graphics objects (so if a default is not explicitly set, the default is this.)
                + 'auto' -- plot the given data limits in the given (or computed) figsize, filling the figure (default for plot and list_plot)
                + number -- ratio of height to width.

To patchbot/reviewers: Apply attachment:trac_2100-aspect-ratio-rebase.patch and attachment:trac_2100-auto-automatic-and-vector.patch

Attachments (6)

2100-aspect_ratio.2.patch (35.7 KB) - added by jason 9 years ago.
apply instead of previous patch
trac_2100-aspect-ratio-rebase.patch (35.8 KB) - added by kcrisman 9 years ago.
Rebase to 4.6.1/4.6.2.alpha0
trac_2100-3d-vector.patch (2.7 KB) - added by kcrisman 9 years ago.
reviewer patch
Before.png (22.1 KB) - added by kcrisman 9 years ago.
What this plot looks like before #2100
After.png (14.8 KB) - added by kcrisman 9 years ago.
What this plot looks like after #2100
trac_2100-auto-automatic-and-vector.patch (10.1 KB) - added by kcrisman 8 years ago.
Apply after rebase patch

Download all attachments as: .zip

Change History (86)

comment:1 Changed 11 years ago by jason

For reference, in Mathematica (see http://reference.wolfram.com/mathematica/ref/AspectRatio.html?q=AspectRatio&lang=en )

  • Graphics objects use Automatic aspect ratio
  • plot and listplot use 1/golden ratio
  • parametric plot uses automatic
  • contour plot uses aspect ratio of 1

Automatic is an aspect ratio determined by the pixels in the plot. It does not distort the plot (i.e., circles are still circles).

comment:2 Changed 11 years ago by mabshoff

Jason,

didn't we fix this recently?

Cheers,

Michael

comment:3 Changed 11 years ago by AlexGhitza

  • Type changed from defect to enhancement

comment:4 Changed 9 years ago by jason

  • Report Upstream set to N/A

#9813 takes care of two cases of these.

Interestingly, from the above list, it seems like the golden ratio is only used for a minority of functions (i.e., "plot" and "list_plot"). So maybe we ought to change the global default to aspect_ratio=1 and just set plot and list_plot to have default golden ratio aspect ratios.

comment:5 Changed 9 years ago by jason

Matplotlib aspect ratio setting Matplotlib adjustable setting Mathematica AspectRatio setting Explanation
'auto' doesn't seem to matter Full Make the current data limits fill the available sized box
'equal' or 1 'box' Automatic Make each pixel be square (aspect ratio 1)

It appears that specifying a number in matplotlib ('box' adjustable) makes the *pixels* have a certain height-to-width ratio (e.g., a circle in the picture will appear to have the given aspect ratio), but in Mathematica, a number specifies the overall ratio of the *image* height-to-width.  Thus the numbers are not directly comparable.

Plus, in matplotlib, you can have the 'datalim' be adjustable, which changes the data limits on your plot to make the pixels have a certain aspect ratio.

comment:6 Changed 9 years ago by jason

Matplotlib's fig_aspect seems to do the sort of thing that mma's aspect ratio number setting does:

http://matplotlib.sourceforge.net/api/figure_api.html#matplotlib.figure.figaspect

comment:7 Changed 9 years ago by jason

  • Cc kcrisman added

comment:8 Changed 9 years ago by jason

One other note: setting bbox_inches='tight' in the savefig method seems to override the figure size specified.

comment:9 Changed 9 years ago by jason

We have two concepts of "aspect ratio" that we'd like to expose to the user.

  • Figure aspect ratio -- controls the final image size, including any text labels, etc.  This will be set using the following options:
    • fig_aspect_ratio
      • number - make an image with the specified aspect ratio (within reason) using the figaspect matplotlib function (which tries to be marginally smart, which actually may be a dumb thing to do...)
      • 'auto' (default) - use bbox_inches='tight' to create a figure that holds the drawn objects
    • figsize
      • single number - use this as a base size for fig_aspect_ratio calculations (i.e., bigger numbers = bigger figures)
      • two numbers - an actual figure size in inches
  • Pixel aspect ratio
    • aspect_ratio -- the aspect ratio of a pixel in the image (or of a unit square in data coordinates, for example).  The default here will change depending on the type of plot
      • 'equal' or 1 -- square pixels.  This will be the default for most things
      • 'auto' -- plot the given data limits in the given (or computed) figsize, filling the figure
      • number -- ratio of width to height (or height to width; I can't remember).  For plot() and list_plot(), this will default to giving a golden ratio aspect ratio
    • aspect_ratio_adjust -- what should we adjust to achieve the desired aspect ratio for the items drawn?  Note that by default, the axes limits are enlarged slightly; to eliminate this, set axes_pad=0
      • 'box' (default) -- the frame axes
      • 'datalim' -- the data limits

What do people think?

comment:10 Changed 9 years ago by jason

Helpful matplotlib mailing list post (read the thread): http://www.mail-archive.com/matplotlib-users@lists.sourceforge.net/msg15623.html

comment:11 Changed 9 years ago by jason

  • Description modified (diff)

Moving the proposal up to the description so we can edit it. Ignore the proposal in the comments now.

comment:12 Changed 9 years ago by jason

  • Description modified (diff)

Update the proposal.

comment:13 Changed 9 years ago by jason

I've attached two files.  First, a work-in-progress rewrite of the aspect ratio stuff.  Second, the plot.py file before this patch was applied.  The plot.py file is the version from 4.6.alpha1 with tickets #9740, #9746, and #4342 applied, in order.

comment:14 follow-up: Changed 9 years ago by jason

  • Authors set to Jason Grout
  • Status changed from new to needs_work

This ticket is still needs_work.  Remaining items include:

  • Updating docs to reflect changes, including
    • Most aspect ratios now default to 1
    • new/changed options:
      •  fig_tight
      • the new definition of aspect_ratio (= 1/old definition)
      • aspect_ratio_adjustable
  • Make GraphicsArray work (likely using the nice features of matplotlib in 1.0 for doing multiple subplots!)

Warning: this patch changes the definition of aspect_ratio.  Before, it was width/height, but now it is height/width.  The new meaning is consistent with matplotlib and Mathematica.

Since the patch now changes a very fundamental thing about Sage (the meaning of aspect ratio), it probably should be put off until a major release.

comment:15 Changed 9 years ago by jason

Also todo: set barchart and scatterplot to have default aspect ratios of 'auto'.

comment:16 in reply to: ↑ 14 Changed 9 years ago by jason

Replying to jason:

Warning: this patch changes the definition of aspect_ratio. Before, it was width/height, but now it is height/width. The new meaning is consistent with matplotlib and Mathematica. Since the patch now changes a very fundamental thing about Sage (the meaning of aspect ratio), it probably should be put off until a major release.

Apparently this statement isn't true. The old aspect_ratio and new aspect_ratio mean the same thing. I was basing the statement on reading the docs. Either the current docs are backwards, or I was misreading them.

So, in short, things are good to go for finishing this patch and getting it into Sage as soon as possible.

Changed 9 years ago by jason

apply instead of previous patch

comment:17 Changed 9 years ago by jason

  • Status changed from needs_work to needs_review

New patch revamping the aspect ratio calculations in Sage (and defaults!)

comment:18 Changed 9 years ago by jason

  • Description modified (diff)

Updating description to reflect changes in the patch.

comment:19 follow-up: Changed 9 years ago by novoselt

It seems to me that set_aspect ratio only accepts "auto" as a string input, while the proposal also lists "equal" as an option.

Would it be possible to extend the functionality so that it is possible to specify either only width or only height of the final figure? I am thinking of using it in conjunction with SageTeX, where it would be natural to ask either for width=\textwidth or height=0.5\texthight and have the second dimension determined automatically based on the actual plot and aspect ratio.

comment:20 Changed 9 years ago by novoselt

  • Status changed from needs_review to needs_work
  • Work issues set to complete documentation

New functions pad_for_tick_labels and adjust_figure_to_contain_bbox are not completely documented.

Are #9740, #9746, and #4342 still prerequisites?

comment:21 Changed 9 years ago by novoselt

Oops, got the answer to the last question already, for some reason they were not crossed in the above comment.

comment:22 Changed 9 years ago by jason

  • Cc mhampton added

This might be good bug-day fodder if people are already working on graphics code.

comment:23 in reply to: ↑ 19 Changed 9 years ago by kcrisman

  • Reviewers set to Andrey Novoseltsev

I hope to take care of novoselt's comments - 'equal' and the documentation - today. This will depend on #7981, #8632, #10244, and #10143.

Would it be possible to extend the functionality so that it is possible to specify either only width or only height of the final figure? I am thinking of using it in conjunction with SageTeX, where it would be natural to ask either for width=\textwidth or height=0.5\texthight and have the second dimension determined automatically based on the actual plot and aspect ratio.

I think this is a good idea, but I don't think the people who might work on this will have time for it before bitrot sets in even further. I have opened ticket #10633 for this.

comment:24 Changed 9 years ago by kcrisman

Rather impressively, only one of seven hunks failed in contour_plot.py, and only 5 of 34 hunks failed on plot.py. So rebasing this to the other recent graphics patches should be no problem, as well as the other documentation comments.

comment:25 Changed 9 years ago by jason

Let's just take the 'equal' option out of the proposal. I think it's there just because that's what matplotlib does.

comment:26 follow-up: Changed 9 years ago by kcrisman

I had to change a few things based on the new save() but otherwise nearly all is the same. It also turned out that fig_tight was never used in .matplotlib(), and in fact only can be passed to matplotlib's savefig(), so I treated it basically the same as transparent in .save().

I can also remove 'equal' if you want, that's fine.

Before I go ahead and update this, though, I have a question. The two functions not documented (pad_for_tick_labels and adjust_figure_to_contain_bbox) only are used in a commented-out line

            # this messes up the aspect ratio!
            #figure.canvas.mpl_connect('draw_event', pad_for_tick_labels)

So... should they even be included at this time?

comment:27 Changed 9 years ago by kcrisman

  • Description modified (diff)

comment:28 in reply to: ↑ 26 ; follow-up: Changed 9 years ago by kcrisman

Replying to kcrisman:

I had to change a few things based on the new save() but otherwise nearly all is the same. It also turned out that fig_tight was never used in .matplotlib(), and in fact only can be passed to matplotlib's savefig(), so I treated it basically the same as transparent in .save().

I can also remove 'equal' if you want, that's fine.

Done as well.

Before I go ahead and update this, though, I have a question. The two functions not documented (pad_for_tick_labels and adjust_figure_to_contain_bbox) only are used in a commented-out line

            # this messes up the aspect ratio!
            #figure.canvas.mpl_connect('draw_event', pad_for_tick_labels)

So... should they even be included at this time?

Quote from Jason: "Feel free to move those functions to another ticket if they are really not called from anywhere. I think they are leftovers from experimentation, so probably not strictly needed (especially if they really aren't called or used from anywhere)."

New patch coming up relatively soon.

comment:29 in reply to: ↑ 28 Changed 9 years ago by kcrisman

So... should they even be included at this time?

Quote from Jason: "Feel free to move those functions to another ticket if they are really not called from anywhere. I think they are leftovers from experimentation, so probably not strictly needed (especially if they really aren't called or used from anywhere)."

Since I didn't really know where these should be used, or what the context was, I decided to just leave them in but fully commented out, with an admonition to add documentation if they ever get used again. (This would also make it easier for a future developer to use them, inter alia.)

Getting some doctest failures, though, related to this:

sage -t  devel/sage/sage/plot/plot3d/plot_field3d.py # 6 doctests failed

I doubt this will be hard to resolve. Anyway, just status updates...

Changed 9 years ago by kcrisman

Rebase to 4.6.1/4.6.2.alpha0

comment:30 follow-up: Changed 9 years ago by kcrisman

  • Authors changed from Jason Grout to Jason Grout, Karl-Dieter Crisman
  • Reviewers changed from Andrey Novoseltsev to Andrey Novoseltsev, Karl-Dieter Crisman
  • Status changed from needs_work to needs_review

The problem with the plot_field3d turns out to be the following very bad behavior of plotting vectors after this patch

sage: v = vector([1.,2.,3.])
sage: v.plot() # all is well
sage: plot(v) # passes in aspect_ratio to a 3d graphics, which already has such a thing, and get nasty traceback

The culprit was telling plot_vector_field3d to do plot(v) for each vector instead of v.plot() as it should have. However, this exposes something else really dumb - namely, that plot.py turns vectors into tuples before it plots them

    from sage.structure.element import is_Vector
    if kwds.get('parametric',False) and is_Vector(funcs):
        funcs = tuple(funcs)


    if hasattr(funcs, 'plot'):
        G = funcs.plot(*args, **original_opts)
    # if we are using the generic plotting method
    else:

this presumably avoids the fact that

sage: w = vector([x^2,3,x^3])
sage: plot(w,(x,0,1))
---------------------------------------------------------------------------
NotImplementedError                   

but still this is a problem. So after fixing this issue, I'm opening a ticket for making vector plotting more sensible.

I think this change might need review (?) so I'm setting to 'needs review'.

To buildbot and reviewer: apply trac_2100-aspect-ratio-rebase.patch and trac_2100-3d-vector.patch. Depends on #7981, #8632, #10244, and #10143.

comment:31 Changed 9 years ago by kcrisman

  • Work issues complete documentation deleted

comment:32 in reply to: ↑ 30 Changed 9 years ago by kcrisman

I think this change might need review (?) so I'm setting to 'needs review'.

Oh yeah, and I never actually looked at very many of the plots. Definitely still needs review ;) of the pictures and the (small) reviewer patch.

comment:33 Changed 9 years ago by novoselt

Depends on #10143

(others are "inherited")

comment:34 follow-up: Changed 9 years ago by novoselt

  • Status changed from needs_review to positive_review

I'm setting this to positive review - it is a great improvement and it is a shame that it took us so long ;-)

I am fine with the reviewer patch and I did take a look at a few (although definitely not all) plots in the documentation. Circles are finally circles!!!

I have also discovered the following behaviour which may be a bug:

sage: x,y = var('x,y') 
sage: print x, y
sage: f(x,y) = x^2 + y^2 - 2 
sage: implicit_plot(f, (-3, 3), (-3, 3),fill=True).show(aspect_ratio=1) 
sage: implicit_plot(f, (-3, 3), (-3, 3),fill=False)

The first line (with fill=True) completely fills the plot, not just the disk.

comment:35 in reply to: ↑ 34 Changed 9 years ago by kcrisman

Thanks!

I have also discovered the following behaviour which may be a bug:

sage: x,y = var('x,y') 
sage: print x, y
sage: f(x,y) = x^2 + y^2 - 2 
sage: implicit_plot(f, (-3, 3), (-3, 3),fill=True).show(aspect_ratio=1) 
sage: implicit_plot(f, (-3, 3), (-3, 3),fill=False)

The first line (with fill=True) completely fills the plot, not just the disk.

This also happens with e.g. 4.6.1.alpha3, so this is definitely not related. It sounds a LOT like #9744. It'd be great if you could confirm that. I thought we'd already taken care of this... unfortunately not, it seems.

comment:36 follow-up: Changed 9 years ago by novoselt

  • Status changed from positive_review to needs_work
  • Work issues set to Plotting of vectors examples

Actually, I was too hasty:

sage -t -long "devel/sage-main/sage/modules/free_module_element.pyx"
**********************************************************************
File "/mnt/usb1/scratch/novoselt/sage/devel/sage-main/sage/modules/free_module_element.pyx", line 1390:
    sage: plot(v) # defaults to an arrow plot
Exception raised:
    Traceback (most recent call last):
      File "/mnt/usb1/scratch/novoselt/sage/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/mnt/usb1/scratch/novoselt/sage/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/mnt/usb1/scratch/novoselt/sage/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_45[9]>", line 1, in <module>
        plot(v) # defaults to an arrow plot###line 1390:
    sage: plot(v) # defaults to an arrow plot
      File "/mnt/usb1/scratch/novoselt/sage/local/lib/python/site-packages/sage/misc/displayhook.py", line 174, in displayhook
        print_obj(sys.stdout, obj)
      File "/mnt/usb1/scratch/novoselt/sage/local/lib/python/site-packages/sage/misc/displayhook.py", line 142, in print_obj
        print >>out_stream, `obj`
      File "base.pyx", line 80, in sage.plot.plot3d.base.Graphics3d.__repr__ (sage/plot/plot3d/base.c:2453)
        self.show()
      File "base.pyx", line 1048, in sage.plot.plot3d.base.Graphics3d.show (sage/plot/plot3d/base.c:9375)
        opts = self._process_viewing_options(kwds)
      File "base.pyx", line 953, in sage.plot.plot3d.base.Graphics3d._process_viewing_options (sage/plot/plot3d/base.c:9174)
        self._determine_frame_aspect_ratio(opts['aspect_ratio'])
      File "base.pyx", line 199, in sage.plot.plot3d.base.Graphics3d._determine_frame_aspect_ratio (sage/plot/plot3d/base.c:3397)
        return [(a_max[i] - a_min[i])*aspect_ratio[i] for i in range(3)]
    TypeError: can't multiply sequence by non-int of type 'float'
**********************************************************************
File "/mnt/usb1/scratch/novoselt/sage/devel/sage-main/sage/modules/free_module_element.pyx", line 1391:
    sage: plot(v, plot_type='arrow')
Exception raised:
    Traceback (most recent call last):
      File "/mnt/usb1/scratch/novoselt/sage/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/mnt/usb1/scratch/novoselt/sage/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/mnt/usb1/scratch/novoselt/sage/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_45[10]>", line 1, in <module>
        plot(v, plot_type='arrow')###line 1391:
    sage: plot(v, plot_type='arrow')
      File "/mnt/usb1/scratch/novoselt/sage/local/lib/python/site-packages/sage/misc/displayhook.py", line 174, in displayhook
        print_obj(sys.stdout, obj)
      File "/mnt/usb1/scratch/novoselt/sage/local/lib/python/site-packages/sage/misc/displayhook.py", line 142, in print_obj
        print >>out_stream, `obj`
      File "base.pyx", line 80, in sage.plot.plot3d.base.Graphics3d.__repr__ (sage/plot/plot3d/base.c:2453)
        self.show()
      File "base.pyx", line 1048, in sage.plot.plot3d.base.Graphics3d.show (sage/plot/plot3d/base.c:9375)
        opts = self._process_viewing_options(kwds)
      File "base.pyx", line 953, in sage.plot.plot3d.base.Graphics3d._process_viewing_options (sage/plot/plot3d/base.c:9174)
        self._determine_frame_aspect_ratio(opts['aspect_ratio'])
      File "base.pyx", line 199, in sage.plot.plot3d.base.Graphics3d._determine_frame_aspect_ratio (sage/plot/plot3d/base.c:3397)
        return [(a_max[i] - a_min[i])*aspect_ratio[i] for i in range(3)]
    TypeError: can't multiply sequence by non-int of type 'float'
**********************************************************************
File "/mnt/usb1/scratch/novoselt/sage/devel/sage-main/sage/modules/free_module_element.pyx", line 1393:
    sage: plot(v, plot_type='point')+frame3d((0,0,0), v.list())
Exception raised:
    Traceback (most recent call last):
      File "/mnt/usb1/scratch/novoselt/sage/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/mnt/usb1/scratch/novoselt/sage/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/mnt/usb1/scratch/novoselt/sage/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_45[12]>", line 1, in <module>
        plot(v, plot_type='point')+frame3d((Integer(0),Integer(0),Integer(0)), v.list())###line 1393:
    sage: plot(v, plot_type='point')+frame3d((0,0,0), v.list())
      File "/mnt/usb1/scratch/novoselt/sage/local/lib/python/site-packages/sage/misc/displayhook.py", line 174, in displayhook
        print_obj(sys.stdout, obj)
      File "/mnt/usb1/scratch/novoselt/sage/local/lib/python/site-packages/sage/misc/displayhook.py", line 142, in print_obj
        print >>out_stream, `obj`
      File "base.pyx", line 80, in sage.plot.plot3d.base.Graphics3d.__repr__ (sage/plot/plot3d/base.c:2453)
        self.show()
      File "base.pyx", line 1048, in sage.plot.plot3d.base.Graphics3d.show (sage/plot/plot3d/base.c:9375)
        opts = self._process_viewing_options(kwds)
      File "base.pyx", line 953, in sage.plot.plot3d.base.Graphics3d._process_viewing_options (sage/plot/plot3d/base.c:9174)
        self._determine_frame_aspect_ratio(opts['aspect_ratio'])
      File "base.pyx", line 199, in sage.plot.plot3d.base.Graphics3d._determine_frame_aspect_ratio (sage/plot/plot3d/base.c:3397)
        return [(a_max[i] - a_min[i])*aspect_ratio[i] for i in range(3)]
    TypeError: can't multiply sequence by non-int of type 'float'
**********************************************************************
1 items had failures:
   3 of  17 in __main__.example_45
***Test Failed*** 3 failures.

These are the only errors on the whole library. I guess they should be fixed in the same way as in the reviewer patch. Or maybe there should be a better fix?..

comment:37 Changed 9 years ago by novoselt

Yes, the problem I hit is exactly #9744 (which actually mentions the same example from the documentation), thanks for pointing it out.

comment:38 in reply to: ↑ 36 ; follow-up: Changed 9 years ago by kcrisman

Replying to novoselt:

Actually, I was too hasty: sage -t -long "devel/sage-main/sage/modules/free_module_element.pyx"

Aargh! I'll try to fix this...

These are the only errors on the whole library. I guess they should be fixed in the same way as in the reviewer patch. Or maybe there should be a better fix?..

No, that is the correct fix for now. An overarching ticket is #10638, which I forgot to mention before.

comment:39 in reply to: ↑ 38 ; follow-up: Changed 9 years ago by kcrisman

Replying to kcrisman:

Replying to novoselt:

Actually, I was too hasty: sage -t -long "devel/sage-main/sage/modules/free_module_element.pyx"

Aargh! I'll try to fix this...

These are the only errors on the whole library. I guess they should be fixed in the same way as in the reviewer patch. Or maybe there should be a better fix?..

No, that is the correct fix for now. An overarching ticket is #10638, which I forgot to mention before.

The correct fix is to remove that kwd from the plot when

        if plot_type == 'arrow' or plot_type == 'point':
            dimension = len(coords)
            if dimension == 3:
                from sage.plot.plot3d.shapes2 import line3d, point3d
                
                if plot_type == 'arrow':
                    return line3d([(0,0,0), coords], arrow_head=True, **kwds)
                else:
                    return point3d(coords, **kwds)

that should do it. I will try this later.

comment:40 in reply to: ↑ 39 Changed 9 years ago by kcrisman

These are the only errors on the whole library. I guess they should be fixed in the same way as in the reviewer patch. Or maybe there should be a better fix?..

No, that is the correct fix for now. An overarching ticket is #10638, which I forgot to mention before.

Okay, now I am not rushing out the door and can think straight. The problem is that this was never doctesting the thing in question in the first place because of the issues raised in #10638.

I think the issue is that _extract_kwds_for_show keeps the @options value of aspect_ratio='auto') in play; when this gets passed to the show() method of Line (which is what line3d creates in this case), it refers to the one in base.PrimitiveObject?, which unfortunately then puts this value of aspect_ratio` in:

        opts = {}
        opts.update(SHOW_DEFAULTS)
        if self._extra_kwds is not None:
            opts.update(self._extra_kwds)
        opts.update(kwds)

and then doesn't realize it's a problem in

        if opts['frame_aspect_ratio'] == 'automatic':
            if opts['aspect_ratio'] != 'automatic':
                # Set the aspect_ratio of the frame to be the same as that of
                # the object we are rendering given the aspect_ratio we'll use
                # for it.
                opts['frame_aspect_ratio'] = \
                    self._determine_frame_aspect_ratio(opts['aspect_ratio'])

Which is where the problem happens.

Although fixing this in the vector plotting didn't seem to work immediately.

Unless we wanted to change all the 'auto' in this patch to 'automatic'. I don't know if that is desirable either, though. What do others think?

comment:41 follow-up: Changed 9 years ago by novoselt

I like auto more than automatic, so I don't want to change it.

Can we maybe change it the other way? Was automatic documented/used somewhere before?

comment:42 in reply to: ↑ 41 Changed 9 years ago by kcrisman

I like auto more than automatic, so I don't want to change it.

Agreed, but...

Can we maybe change it the other way? Was automatic documented/used somewhere before?

Yes, this is the standard 3D option. See this reference manual page for plot/plot3d/base.pyx.

Of course, it would be possible to change the plot3d case to also accept 'auto' easily enough - in fact, that's essentially what I've done in my latest patch - and one could change the documentation accordingly. But we couldn't remove it without a deprecation period - and in my mind it's pointless to go to the trouble, given that it's mostly a default so we could change things without even needing to deprecate that option.

comment:43 follow-up: Changed 9 years ago by novoselt

I guess then changing to automatic is OK.

For consistency, it would be nice if the same variant was used throughout Sage, but then again the point is that this value should not be specified explicitly by the user.

comment:44 in reply to: ↑ 43 ; follow-up: Changed 9 years ago by kcrisman

Replying to novoselt:

I guess then changing to automatic is OK.

I'd want to run that by Jason (the original author) first, though - after all, the point below would suggest it doesn't matter (and it is a long process for me to rebase, sadly).

For consistency, it would be nice if the same variant was used throughout Sage, but then again the point is that this value should not be specified explicitly by the user.

comment:45 in reply to: ↑ 44 Changed 9 years ago by kcrisman

I guess then changing to automatic is OK.

I'd want to run that by Jason (the original author) first, though - after all, the point below would suggest it doesn't matter (and it is a long process for me to rebase, sadly).

One reason being that matplotlib uses 'auto', and consistency with that is nice as well.

For consistency, it would be nice if the same variant was used throughout Sage, but then again the point is that this value should not be specified explicitly by the user.

comment:46 Changed 9 years ago by kcrisman

  • Status changed from needs_work to needs_review

Okay, this latest reviewer patch should fix the things we've talked about in a minimalist way. It doesn't resolve the 'auto' versus 'automatic' issue. No new instructions for applying patches. Documentation also looks good with this change.

Changed 9 years ago by kcrisman

reviewer patch

comment:47 Changed 9 years ago by kcrisman

  • Work issues Plotting of vectors examples deleted

comment:48 follow-up: Changed 9 years ago by jason

Wow, great job tracking down this subtle problem. I personally slightly prefer 'auto' over 'automatic' because it's consistent with matplotlib and it's shorter. However, I agree that users will probably not specify it too frequently, so 'automatic' would probably be fine as well. If we do make the default 'automatic', then I think we'll have to convert it to 'auto' before passing it to matplotlib.

Another data point: mma uses Automatic (not Auto): http://reference.wolfram.com/mathematica/ref/Automatic.html

comment:49 in reply to: ↑ 48 Changed 9 years ago by kcrisman

Replying to jason:

Wow, great job tracking down this subtle problem. I personally slightly prefer 'auto' over 'automatic' because it's consistent with matplotlib and it's shorter. However, I agree that users will probably not specify it too frequently, so 'automatic' would probably be fine as well.

Thanks. I don't have any more time today to work on this, but I think then I vote for 'automatic' for consistency, and having 'auto' as one that secretly also works :)

If we do make the default 'automatic', then I think we'll have to convert it to 'auto' before passing it to matplotlib.

Yes, I realize that. I don't think that will be very difficult, since there seems to be only one place this is passed.

comment:50 Changed 9 years ago by kcrisman

  • Status changed from needs_review to needs_work
  • Work issues set to unify option names

Okay, putting this to 'needs work' in order to fix this. Hopefully today or this weekend; it won't be hard, just have to do it.

comment:51 Changed 9 years ago by kcrisman

Oops... hmm, I gave myself one long weekend. I'll try to see if this still applies to 4.6.2.alpha4 soon.

comment:52 Changed 9 years ago by kcrisman

All still applies to 4.6.2.alpha4. I hope to conclude this adventure shortly, by replacing the reviewer patch with one that deals with the automatic/auto issue uniformly.

comment:53 follow-up: Changed 9 years ago by kcrisman

  • Description modified (diff)
  • Status changed from needs_work to needs_review

This should all work fine now. I do want to point out that the standard plots seem to have increased somewhat in size. The new reviewer patch is now different enough that it needs review. Please do so now to avoid bitrot!

comment:54 in reply to: ↑ 53 Changed 9 years ago by jason

Replying to kcrisman:

This should all work fine now. I do want to point out that the standard plots seem to have increased somewhat in size.

Yes, I reverted things to the matplotlib defaults. Before, we made the ratio of height/width the golden ratio, but now it is the standard matplotlib default (which I believe is 4:3, which matches computer screens better anyway).

comment:55 Changed 9 years ago by jason

  • Owner changed from was to jason

Can you add instructions for reviewers/patchbot about which patches should be applied in what order?

comment:56 Changed 9 years ago by jason

(and then I'll try to review this tonight or tomorrow)

comment:57 Changed 9 years ago by kcrisman

  • Description modified (diff)

See the description. I guess I didn't make it obvious enough :)

Apply [trac_2100-aspect-ratio-rebase.patch] and [trac_2100-auto-automatic-and-vector.patch] for this.

comment:58 follow-up: Changed 9 years ago by kcrisman

  • Description modified (diff)

Apply [trac_2100-aspect-ratio-rebase.patch trac_2100-aspect-ratio-rebase.patch] and [trac_2100-auto-automatic-and-vector.patch trac_2100-auto-automatic-and-vector.patch] for this.

comment:59 Changed 9 years ago by kcrisman

I can't get the links to work for some reason - sorry.

comment:60 in reply to: ↑ 58 Changed 9 years ago by kcrisman

  • Description modified (diff)

comment:61 Changed 9 years ago by kcrisman

Here is some code that looks less good with this patch than before.

def my_eulers_method_plot(a_function,x0,y0,h,x1):
    n=int((1.0)*(x1-x0)/h)
    x00=x0; y00=y0
    x01=x0; y01=y0
    P=point((x00,y00),rgbcolor=hue(1)) # red    
    Q=Graphics() # default is blue
    f(x,y)=a_function(x,y) # Note that a_function should be a callable function of x, then y
    for i in range(n+1):
        y01 = y00+h*f(x00,y00)
        x01 = x00+h
        P=P+point((x01,y01),rgbcolor=hue(1))
        Q=Q+line([(x00,y00),(x01,y01)])
        x00=x01
        y00=y01
    return(P+Q)

var('x,y')
def euler_logistic_plot(parameter,y_start,end=15,step=1):
    function(x,y)=parameter*y*(1-y)
    my_eulers_method_plot(function,0,y_start,step,end).show(ymin=0)

euler_logistic_plot(2,.7,97,.8)

I'll try to attach before and after.

Changed 9 years ago by kcrisman

What this plot looks like before #2100

Changed 9 years ago by kcrisman

What this plot looks like after #2100

comment:62 Changed 9 years ago by kcrisman

  • Status changed from needs_review to needs_info

The issue is of course that I never used plot() to do this, but rather only line() and point() and Graphics(). The default aspect ratio seems to have changed a lot, and so not only are the labels on the left totally squished, but it just looks bad. Maybe mpl defaults aren't that great? Or is this a pretty unusual scenario?

Anyway, putting as needs info until I know what is happening here from someone who knows more about mpl (e.g., Jason). So frustrating, too, because we should have this in!

comment:63 Changed 9 years ago by jason

Yep, I think the problem here is that line and point now default to have aspect_ratio=1, so the y and x axis are drawn at equal scales. Try setting the aspect_ratio to 'auto' (or whatever your patch does now).

plot() defaults to 'auto' aspect ratio (e.g., fill the figure). However, geometric figures like line and point default to aspect_ratio=1.

comment:64 Changed 9 years ago by kcrisman

I think perhaps we should make line (and point?) default to 'auto'? Because one is quite likely to have a line do a function, as opposed to other geometric figures. And points are often used to shadow functions.

Any other thoughts on this? It would be really nice to finally get this in 4.7 - maybe you can take a quick look over spring break?

comment:65 Changed 9 years ago by kcrisman

  • Status changed from needs_info to needs_review
  • Work issues unify option names deleted

Putting needs review, though may need a minor change because of the thing in the last two comments.

comment:66 Changed 8 years ago by kcrisman

  • Keywords sd31 added

Okay, I'm going to refresh so that line and point still have 'auto'. Needs review!

comment:67 Changed 8 years ago by ryan

  • Reviewers changed from Andrey Novoseltsev, Karl-Dieter Crisman to Andrey Novoseltsev, Karl-Dieter Crisman, Ryan Grout

FYI, I am currently reviewing this patch

comment:68 follow-up: Changed 8 years ago by ryan

  • Status changed from needs_review to positive_review

Patch looks good. Confirmed that kcrisman's code looks 'good' after the patch too :) Positive Review. Great job guys.

Note: I do get doctest failures on plot_field.py, but the failures are not caused by the patches to this ticket.

comment:69 in reply to: ↑ 68 Changed 8 years ago by novoselt

Replying to ryan:

Note: I do get doctest failures on plot_field.py, but the failures are not caused by the patches to this ticket.

What exactly do you mean by this? That a clean public release of Sage has doctest failures in plot_field.py?

comment:70 Changed 8 years ago by kcrisman

Correct - or to be more precise, warnings that do not cause the doctest to fail. They are known, and not from Sage.

comment:71 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-4.7.1 to sage-4.7.2

comment:72 follow-up: Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work
  • Work issues set to rebase

Please rebase this patch to be applied on top of #11491.

comment:73 in reply to: ↑ 72 ; follow-up: Changed 8 years ago by kcrisman

Replying to jdemeyer:

Please rebase this patch to be applied on top of #11491.

Thanks - another rebase where a major improvement takes second rank to a trivial adjustment. Especially since Ryan and I were involved on both tickets, it would have been nice to ask first. Rebasing is not so trivial for the setups some of us have.

But no point crying over spilt milk. Sigh... patches coming up.

Changed 8 years ago by kcrisman

Apply after rebase patch

comment:74 Changed 8 years ago by kcrisman

  • Status changed from needs_work to positive_review

Okay, now we should hopefully be at positive review!

comment:75 in reply to: ↑ 73 Changed 8 years ago by jdemeyer

Replying to kcrisman:

Especially since Ryan and I were involved on both tickets, it would have been nice to ask first.

I have done that in the past and people complained: http://groups.google.com/group/sage-devel/browse_frm/thread/abd5fb944769e052/8e4057172b97f2e5

comment:76 follow-ups: Changed 8 years ago by kcrisman

Right, and my point is that this sort of feels the same way. I am not suggesting unmerging something positively reviewed - definitely not the direction I'm going.

What I'm suggesting (and only suggesting, because I can only imagine how much work release management is) is that perhaps sending a ping via Trac on two closely related tickets which have some overlap would be good before deciding which one to merge first. Especially since in this case #11491 is so clearly much more trivial than this one, and also much older. I'm sure Ryan and I would have agreed that this one was higher priority.

Anyway, it turned out I had time for fixing it - it was a very small overlap, luckily. Thanks for all the hard work; the final releases really are noticeably more polished nowadays, and it's a great thing.

comment:77 in reply to: ↑ 76 ; follow-up: Changed 8 years ago by jdemeyer

  • Work issues rebase deleted

Replying to kcrisman:

Right, and my point is that this sort of feels the same way. I am not suggesting unmerging something positively reviewed - definitely not the direction I'm going.

What I'm suggesting (and only suggesting, because I can only imagine how much work release management is) is that perhaps sending a ping via Trac on two closely related tickets which have some overlap would be good before deciding which one to merge first. Especially since in this case #11491 is so clearly much more trivial than this one, and also much older. I'm sure Ryan and I would have agreed that this one was higher priority.

It is not easy for me to determine a priori which patches conflict with eachother, so what you propose isn't really possible. It's not that I looked at both patches and decided which one to merge. What happened is that I already decided to merge #11491 before I even considered the patch here.

Since the sage-devel discussion I mentioned, there is an agreement that once a patch is merged (even in a future alpha version), it should stay merged if possible.

comment:78 in reply to: ↑ 76 Changed 8 years ago by jdemeyer

  • Cc mpatel added

Replying to kcrisman:

Thanks for all the hard work; the final releases really are noticeably more polished nowadays, and it's a great thing.

You should really also thank Mitesh Patel for managing the buildbot.

comment:79 in reply to: ↑ 77 Changed 8 years ago by kcrisman

It is not easy for me to determine a priori which patches conflict with eachother, so what you propose isn't really possible. It's not that I looked at both patches and decided which one to merge. What happened is that I already decided to merge #11491 before I even considered the patch here.

Understood.

Since the sage-devel discussion I mentioned, there is an agreement that once a patch is merged (even in a future alpha version), it should stay merged if possible.

Yes, that definitely makes sense, as I hope I make clear above. :)

comment:80 Changed 8 years ago by jdemeyer

  • Merged in set to sage-4.7.2.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.