Skip to content

Equal area markers #16891

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Equal area markers #16891

wants to merge 7 commits into from

Conversation

brunobeltran
Copy link
Contributor

@brunobeltran brunobeltran commented Mar 23, 2020 •
edited
Loading

PR Summary

WIP. A proposal to resolve #15703.

In short, I propose adding one new parameter to MarkerStyle.__init__, and expanding the options currently available for normalization (copied from the proposed docstring of MarkerStyle.__init__):

        normalization : str, optional, default: "classic"
            The normalization of the marker size. Can take several values:
            *'classic'*, being the default, makes sure custom marker paths are
            normalized to fit within a unit-square by affine scaling (but
            leaves built-in markers as-is).
            *'bbox-width'*, ensure marker path fits in the unit square.
            *'area'*, rescale so the marker path has unit "signed_area".
            *'bbox-area'*, rescale so that the marker path's bbox has unit
            area.
            *'none'*, in which case no scaling is performed on the marker path.

        centering : str, optional, default: "classic"
            The centering of the marker. Can take several values:
            *'none'*, being the default, does not translate the marker path.
            The origin in path coordinates is the marker center in this case.
            *'bbox'*, translates the marker path so that its bbox's center is
            at the origin.
            *'center-of-mass'*, translates the marker path so that its center
            of mass it as the origin. See Path.center_of_mass for details.

Goals

A "complete" custom marker system, that allows the user to choose between several options for marker normalization and centering as is most relevant to their particular plotting goals.

Implementation

The marker customization options will require the following new functionality

  1. Path.get_exact_extents (a bugfix of _path.get_extents does not correctly handle bezier curves #16830, implemented in Correctly compute path extents #16832)
  2. Path.length (Path length #16888)
  3. Path.signed_area (Compute area of path #16859)
  4. Path.center_of_mass (Path center of mass #16889)

Any reasonable implementation of the above three methods will require

  1. BezierSegment.arc_length (Path length #16888)
  2. BezierSegment.arc_area (Compute area of path #16859)
  3. BezierSegment.center_of_mass (Path center of mass #16889)

Each Path function will simply accumulate the values for its BezierSegment counterpart to compute the values for the total path (our Path's right now are simply collections of BezierSegments, even though they're not stored that way explicitly).

Unfortunately, for methods of Path to call methods of BezierSegment, some slight re-arrangements are required to prevent circular import issues (#16812).

Finally, in order to iterate easily over the BezierSegment's that make up the path (to do this accumulation), I'll need to implement

  1. Path.iter_bezier (Bezier/Path API Cleanup: fix circular import issue #16812)

Using iter_segments directly instead requires a HUGE amount repeated code. Incorporating the proposed .iter_bezier functionality into iter_segments leads to an absolutely unwieldy amount of bloat in iter_segments, so the real question I think is just whether this should be added to the public API (it seems like a very good candidate to me, as it produces MUCH cleaner user code than using iter_segments for any code I've had to write, and presents a much more simple model of the underlying Path).

Roadmap

#16812 (*) <- #16832 (*) <- #16859 (*) <- #16888 <- #16889 (*) <- (This PR)

"<-" means "depends on", and "(*)" marks PRs whose implementations are complete and fully ready for review.

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

This was referenced Mar 23, 2020
Merged
Draft
Open
Open
Merged
@tacaswell tacaswell added this to the v3.3.0 milestone Mar 23, 2020
@brunobeltran brunobeltran mentioned this pull request Mar 23, 2020
Closed
6 tasks
@jklymak
Copy link
Member

jklymak commented Mar 24, 2020

Can we use this PR to talk about the proposed API and the need for it? There are so many other issues and PRs that I am having trouble following what has been said where.

I'd really appreciate a few examples as to what this new feature is supposed to look like to the end user, and a bit more discussion as to whether this is worth the complexity. It seems there is one use case where its "lets let the user normalize the unit size of markers given a limited set of options" all the way to "lets let the user arbitrarily transform markers using the full transform stack". Perhaps these are not incompatible, but it would be nice to see what this will look like by the end of all these PRs.

Right now the only example I've seen in any of the PRs is the quite esoteric desire to lay out pies charts using scatter, which is pretty gee-whiz, but doesn't strike me as a good argument for any of this functionality. Are there examples that show the power of this approach for actual scatter plots?

@brunobeltran
Copy link
Contributor Author

brunobeltran commented Mar 24, 2020 •
edited
Loading

@jklymak absolutely! Opened this PR to centralize discussion since, as you say, it was pretty fractured before.

I'll let @ImportanceOfBeingErnest speak for the use cases for custom paths for a marker, but we already allow custom paths for markers, that is existing API.

This PR is not about allowing custom markers, but allowing us to normalize markers so that they look perceptually uniform (#15703).

Since we already allow custom markers, any marker normalization code has to be able to perform its duties when passed an arbitrary path. Thankfully, the formulas for the area/extents/etc of an arbitrary path are actually quite simple (thanks to the fact that our Path objects are just a list of bezier curves), so this PR solves the normalization problem by just implementing the code to compute the area/bbox area/etc of an arbitrary path.

Is the issue that you don't see a use-case for custom markers? We'd have to remove existing 3.2.x API to get rid of them. (Although the changes in this PR would go in more or less as-is regardless).

@ImportanceOfBeingErnest
Copy link
Member

There is a section "Why is this useful?" in my PR #16773.

@mwaskom
Copy link

mwaskom commented May 16, 2020

Just found this issue and wanted to offer my encouragement. It looks like it's going to take substantial effort, but I think it would be a really big improvement.

@tacaswell tacaswell modified the milestones: v3.3.0, v3.4.0 May 23, 2020
@jklymak jklymak marked this pull request as draft July 23, 2020 03:19
@brunobeltran brunobeltran mentioned this pull request Dec 23, 2020
Closed
6 tasks
@QuLogic QuLogic modified the milestones: v3.4.0, v3.5.0 Jan 21, 2021
@QuLogic QuLogic modified the milestones: v3.5.0, v3.6.0 Aug 18, 2021
@timhoffm timhoffm modified the milestones: v3.6.0, unassigned Apr 30, 2022
@story645 story645 modified the milestones: unassigned, needs sorting Oct 6, 2022
@github-actions
Copy link

Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it.

@github-actions github-actions bot added the status: inactive Marked by the “Stale” Github Action label Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
status: inactive Marked by the “Stale” Github Action status: needs rebase status: waiting for other PR status: work in progress
Projects
None yet
Milestone
future releases
Development

Successfully merging this pull request may close these issues.

Sizes of different markers are not perceptually uniform
8 participants
@brunobeltran @jklymak @ImportanceOfBeingErnest @mwaskom @tacaswell @QuLogic @story645 @timhoffm