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

POC: Compound marks #3120

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

POC: Compound marks #3120

wants to merge 4 commits into from

Conversation

mwaskom
Copy link
Owner

@mwaskom mwaskom commented Oct 30, 2022

This PR has a proof of concept for a "compound mark" concept (cf #2991). In this implementation, marks are combined through the addition operator (e.g. Dot() + Range()):

(
    so.Plot(glue, "Score", "Model")
    .add(so.Dot() + so.Range(), so.Est())
)

A potentially useful (but also potentially surprising) behavior is that the second mark will inherit any properties set directly on the first mark:

(
    so.Plot(glue, "Score", "Model")
    .add(so.Dot(color=".15") + so.Range(), so.Est())
)

(We could decide that this is too magical to be helpful).

Mark properties are mapped together:

(
    so.Plot(glue, "Score", "Model", color="Encoder")
    .add(so.Dot() + so.Range(), so.Est())
)

Compound marks also move together and simplify some existing edge cases (cf. #2987)

(
    so.Plot(glue, "Score", "Encoder", color="Model")
    .add(so.Bar(width=.5) + so.Range(), so.Est(), so.Dodge(empty="fill"))
)

Open questions

Is overloading Mark.__add__ the right approach here? It would be the only place where a mathematical operator is overloaded to build a plot. And we need to be mindful of the adjacency to ggplot syntax, which uses + overloading for everything. There's also potentially the odd tension of having this operation happening within a method called .add.

Alternatives considered:

  • Make CompoundMark a public object and use it, possibly setting common properties with kwargs, e.g. CompoundMark(Dot(), Range(), color="r")
  • Accept a list in Plot.add with the same general logic, e.g. Plot().add([Dot(), Range()], Est())
  • Change the Plot.add signature to be *args and then work out what's what based on types e.g. Plot().add(Dot(), Range(), Est(), Dodge())

My current thinking is that these are, in turn, too verbose / too fussy with brackets / too opaque when read. But I'm not completely sold on addition overloading...

Needs

  • Decision on spelling
  • Decision on "property inheritance"
  • Unit tests
  • Documentation
  • Release note

@codecov
Copy link

codecov bot commented Oct 30, 2022

Codecov Report

Merging #3120 (1be81dc) into master (8dc5a9c) will decrease coverage by 0.09%.
The diff coverage is 56.36%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3120      +/-   ##
==========================================
- Coverage   98.41%   98.32%   -0.10%     
==========================================
  Files          76       76              
  Lines       24076    24117      +41     
==========================================
+ Hits        23695    23712      +17     
- Misses        381      405      +24     
Impacted Files Coverage Δ
seaborn/_marks/base.py 84.33% <42.85%> (-14.07%) ⬇️
seaborn/_marks/area.py 96.51% <100.00%> (ø)
seaborn/_marks/bar.py 100.00% <100.00%> (ø)
seaborn/_marks/dot.py 100.00% <100.00%> (ø)
seaborn/_marks/line.py 100.00% <100.00%> (ø)
seaborn/_marks/text.py 100.00% <100.00%> (ø)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant