Opened 2 years ago

Closed 2 years ago

#20924 closed defect (fixed)

Error in plot - force use of aspect ratio

Reported by: aram.dermenjian Owned by:
Priority: major Milestone: sage-7.4
Component: graphics Keywords:
Cc: kcrisman, egunawan, tscrim, alauve, paulmasson Merged in:
Authors: Aram Dermenjian Reviewers: Paul Masson, Aaron Lauve, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: e93e592 (Commits) Commit: e93e5928f4d0b8db0b28d84838994d0aea686d70
Dependencies: Stopgaps:

Description

It seems as though when using plot() we are forced to have an aspect ratio of automatic whether we want that or not. Example:

If we define a class

class something:
    def plot(self):
        G = Graphics()
        G.set_aspect_ratio(1)
        return G

when we do something.plot() the aspect ratio is correctly set to 1. But when we do plot(something) the aspect ratio gets converted to automatic. This is due to the native way that plot is handled:

file: src/sage/plot/plot.py

@options(alpha=1, thickness=1, fill=False, fillcolor='automatic', fillalpha=0.5, rgbcolor=(0,0,1), plot_points=200, adaptive_tolerance=0.01, adaptive_recursion=5, detect_poles = False, exclude = None, legend_label=None, __original_opts=True, aspect_ratio='automatic')
def plot(funcs, *args, **kwds):
      G_kwds = Graphics._extract_kwds_for_show(kwds, ignore=['xmin', 'xmax'])
  
      original_opts = kwds.pop('__original_opts', {})
      do_show = kwds.pop('show',False)
  
      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:
          # Other things - deleted for clarity
  
      G._set_extra_kwds(G_kwds)
      if do_show:
          G.show()
      return G

Notice how the last option in @options is aspect_ratio='automatic'. The reason that this is the only one with an issue is the comination of Graphics._extract_kwds_for_show and G._set_extra_kwds.

When using Graphics._extract_kwds_for_show we are doing the following: file: src/sage/plot/graphics.py

    SHOW_OPTIONS = dict(# axes options
                        axes=None, axes_labels=None, axes_labels_size=None,
                        axes_pad=None, base=None, scale=None,
                        xmin=None, xmax=None, ymin=None, ymax=None,
                        # Figure options
                        aspect_ratio=None, dpi=DEFAULT_DPI, fig_tight=True,
                        figsize=None, fontsize=None, frame=False,
                        title=None, title_pos=None, transparent=False,
                        # Grid options
                        gridlines=None, gridlinesstyle=None,
                        hgridlinesstyle=None, vgridlinesstyle=None,
                        # Legend options
                        legend_options={}, show_legend=None,
                        # Ticks options
                        ticks=None, tick_formatter=None, ticks_integer=False,
                        # Text options
                        typeset='default')

    def _extract_kwds_for_show(cls, kwds, ignore=[]):
        result = {} 
        for option in cls.SHOW_OPTIONS:
            if option not in ignore:
                try:
                    result[option] = kwds.pop(option)
                except KeyError:
                    pass
        return result

As you can see, we always grab these additional kwds, in particular we grab the 'aspect_ratio' kwd. This means that when G._set_extra_kwds gets called, the aspect ratio is changing AFTER we have already set it in our something.plot() method.

It seems as though this is not working as expected and/or there is some non-intuitive way to get around this issue.

Recommendations:

  • We should only be doing _set_extra_kwds if we are NOT using the plot method of an object
  • Or we should have some way to override this property so that if we are setting an aspect ratio, then it does not get forcibly overwritten as is happening now.

I'm thinking of doing the second one, where I do something of the following nature:

      if hasattr(funcs, 'plot'):
          G = funcs.plot(*args, **original_opts)
          for ext in G._extra_kwds 
              if ext in G_kwds:
                   delete G_kwds[ext]

Like this, if we are ever setting something beforehand, we are not unintentionally removing it as we are now. Thoughts would be beneficial on this ticket as plot is a fairly common tool that is used in multiple places and so I don't want to alter something that might break other things especially if there does actually exist something that would get around this issue that I don't know about and/or if someone has a different idea of how to approach it.

Thank you.

Change History (15)

comment:1 Changed 2 years ago by tscrim

  • Cc kcrisman added

Here's a minimal example of this behavior:

sage: G = graphs.PetersenGraph()
sage: p = G.plot()
sage: p.aspect_ratio()
1.0
sage: pp = plot(G)
sage: pp.aspect_ratio()
'automatic'

Although the plot for graph seems to have some other issues in that it ignores, e.g., aspect_ratio.

comment:2 Changed 2 years ago by egunawan

  • Cc egunawan tscrim added

comment:3 Changed 2 years ago by aram.dermenjian

  • Branch set to u/aram.dermenjian/error_in_plot___force_use_of_aspect_ratio

comment:4 Changed 2 years ago by aram.dermenjian

  • Commit set to 7d99aa5b0ec3b471b216cb0216b1f29ceca3d47a
  • Status changed from new to needs_review

New commits:

7d99aa5Update plot to keep information from extra kwds instead of overwriting them

comment:5 Changed 2 years ago by tscrim

  • Cc alauve paulmasson added
  • Milestone changed from sage-7.3 to sage-7.4

You will need to add your (real) name to the authors' fields, and I think you should add a doctest showing the behavior has changed by looking at the aspect_ratio of a plot (say, the MWE (minimal working example) above).

comment:6 Changed 2 years ago by git

  • Commit changed from 7d99aa5b0ec3b471b216cb0216b1f29ceca3d47a to c369240b164cb3a206e7f11c095af7a86afa1404

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

3177364Merge branch 'develop' into t/20924/error_in_plot___force_use_of_aspect_ratio
c369240Add test for aspect ratio problem

comment:7 Changed 2 years ago by aram.dermenjian

  • Authors set to Aram Dermenjian

I've gone ahead and merged with the latest develop, as well as added the doctest.

comment:8 Changed 2 years ago by paulmasson

  • Reviewers set to Paul Masson
  • Status changed from needs_review to positive_review

This is a nice clean way to preserve keywords. Works as expected for objects with plot methods.

Doctests pass and documentation builds. Looks good to me unless Travis has something else to add.

comment:9 Changed 2 years ago by alauve

grammar: "If we already have certain items already set," --> delete the first "already"

zap whitespace: I notice one line with lots of extra space characters on it. perhaps there are more.

comment:10 Changed 2 years ago by paulmasson

  • Status changed from positive_review to needs_review

Positive review from me when everyone else is ready.

comment:11 Changed 2 years ago by tscrim

@aram.dermenjian Thank you

One other small nitpick: It would be good to also replace kwds -> keywords in the explanation.

comment:12 Changed 2 years ago by git

  • Commit changed from c369240b164cb3a206e7f11c095af7a86afa1404 to e93e5928f4d0b8db0b28d84838994d0aea686d70

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

e93e592Remove unneeded whitespace and grammatical changes

comment:13 Changed 2 years ago by aram.dermenjian

The desired changes have been made (Fixed grammar, fixed the extra white space line, and expanded kwds to keywords) Let me know if there are any other things needing to be changed.

comment:14 Changed 2 years ago by tscrim

  • Reviewers changed from Paul Masson to Paul Masson, Aaron Lauve, Travis Scrimshaw
  • Status changed from needs_review to positive_review

LGTM. Thanks.

comment:15 Changed 2 years ago by vbraun

  • Branch changed from u/aram.dermenjian/error_in_plot___force_use_of_aspect_ratio to e93e5928f4d0b8db0b28d84838994d0aea686d70
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.