toyplot icon indicating copy to clipboard operation
toyplot copied to clipboard

Point extents func to fit marker size + stroke

Open eaton-lab opened this issue 2 years ago • 6 comments

I updated the .extents() for Point Marks to return extents of its markers as msize / 2 + stroke, unless marker type is "rNxM", where it gets height and width from the marker. I'm not sure if there are other non-symmetric markers that need to be accommodated. I've had something like this implemented in toytree for a while and it seems to work well. Thought it would be good to share. I believe this addresses Issue #164

BEFORE

image

AFTER

image

CONFIRMATION

This shows the new extents seem quite accurate by suppressing the padding and margin: image

eaton-lab avatar Apr 22 '23 19:04 eaton-lab

Note: Since the regression test compares against reference plots, I'm not sure this commit is truly failing, since it is suggesting a change that should change many of the plots by slightly extending the domain range. But I'm not very familiar with this testing method.

eaton-lab avatar Apr 22 '23 22:04 eaton-lab

@eaton-lab - I like where this is headed, but this isn't just a case where the reference images need to be updated, the code is failing at render time ... to take one example:

$ behave -i legends -n "@1.6"

Scenario Outline: Render legends -- @1.6                               # features/legends.feature:16
    Given a default canvas                                               # features/steps/common.py:13 0.001s
    And a set of cartesian axes                                          # features/steps/common.py:18 0.001s
    And the data is rendered as scatterplots                             # features/steps/legends.py:41 0.005s
    And the marks are used to create a legend                            # features/steps/legends.py:46 0.003s
    Then the figure should match the legend-scatterplots reference image # features/steps/common.py:28 0.006s
      Traceback (most recent call last):
        File "/Users/tshead/miniconda3/envs/toyplot/lib/python3.10/site-packages/behave/model.py", line 1329, in run
          match.run(runner.context)
        File "/Users/tshead/miniconda3/envs/toyplot/lib/python3.10/site-packages/behave/matchers.py", line 98, in run
          self.func(context, *args, **kwargs)
        File "features/steps/common.py", line 31, in step_impl
          testing.assert_canvas_equal(context.canvas, reference, show_diff=show_diff)
        File "/Users/tshead/src/toyplot/features/steps/testing.py", line 240, in assert_canvas_equal
          sys.modules[module].render(canvas, stream)
        File "/Users/tshead/src/toyplot/toyplot/html.py", line 333, in render
          _render(canvas, context.copy(parent=root_xml)) # pylint: disable=no-value-for-parameter
        File "/Users/tshead/miniconda3/envs/toyplot/lib/python3.10/site-packages/multipledispatch/dispatcher.py", line 278, in __call__
          return func(*args, **kwargs)
        File "/Users/tshead/src/toyplot/toyplot/html.py", line 805, in _render
          _render(node._finalize(), context.copy(parent=svg_xml))
        File "/Users/tshead/src/toyplot/toyplot/coordinates.py", line 813, in _finalize
          (x, y), (left, right, top, bottom) = child.extents(["x", "y"])
      ValueError: too many values to unpack (expected 2)

After poking at it a bit, it looks like toyplot.mark.Point.extents() is returning coordinates with too many dimensions (4 instead of 2) ... I think the problem is around toyplot/mark.py line 1017. Could you take a look?

Cheers, Tim

tshead2 avatar Apr 24 '23 21:04 tshead2

@tshead2 I see, I think this commit will fix it.

But, I think some recent change on main has broken the regression testing framework. It is failing during sudo apt-get install ffmpeg ghostscript, which is obviously not related to this commit.

eaton-lab avatar May 25 '23 17:05 eaton-lab

Yeah, I'm testing the pull-request locally right now. The errors look more like a server offline or something.

Cheers, Tim

tshead2 avatar May 25 '23 18:05 tshead2

Hey @tshead2 ,

Let me know what you think of these regression tests. It seems the only differences are very small changes in transform tags. Is this the expected outcome of expanding the extents for markers?

Deren

eaton-lab avatar Jun 13 '23 13:06 eaton-lab

Closing-and-reopening this PR to trigger re-running the tests.

tshead2 avatar Jul 12 '24 06:07 tshead2