seaborn icon indicating copy to clipboard operation
seaborn copied to clipboard

POC: Compound marks

Open mwaskom opened this issue 3 years ago • 1 comments

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

mwaskom avatar Oct 30 '22 21:10 mwaskom

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%) :arrow_down:
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%> (ø)

codecov[bot] avatar Oct 30 '22 21:10 codecov[bot]