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

Point extents func to fit marker size + stroke #211

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

eaton-lab
Copy link
Contributor

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
Copy link
Contributor Author

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.

@tshead2
Copy link
Member

tshead2 commented Apr 24, 2023

@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

@eaton-lab
Copy link
Contributor Author

eaton-lab commented May 25, 2023

@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.

@tshead2
Copy link
Member

tshead2 commented May 25, 2023

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

Cheers,
Tim

@eaton-lab
Copy link
Contributor Author

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

@tshead2
Copy link
Member

tshead2 commented Jul 12, 2024

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

@tshead2 tshead2 closed this Jul 12, 2024
@tshead2 tshead2 reopened this Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants