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

Differentiate Line/Path and add Lines/Paths alternatives #2822

Merged
merged 5 commits into from
May 30, 2022
Merged

Conversation

mwaskom
Copy link
Owner

@mwaskom mwaskom commented May 28, 2022

There are two API decisions here that I am not 100% confident about:

  • Line vs Path for sorted vs. unsorted, with Path also preserving nulls (showing a break in the path) but Line dropping them.
  • Line vs Lines (and equivalent) for using a Line2D artist vs. a LineCollection.

One alternative would have been to have a single Line mark with a sort: bool parameter, and perhaps a collection: bool parameter. I think it makes sense to expose functionality more obviously, and to try and limit mark parameters to mappables? Although, at some point the number of distinct marks would get out of control and it would be hard to find one you are looking for.

The other weird thing about this convention is that many singular-named marks that draw with collections and hence are pretty fast. So the rule becomes something like; when there is a singular/plural pair, it implies that the singular mark is slow (but more fully featured or easier to work with as an artist) but a singular mark itself does not imply slowness? Kind of messy.

@mwaskom mwaskom added this to the v0.12.0 milestone May 28, 2022
@codecov
Copy link

codecov bot commented May 28, 2022

Codecov Report

Merging #2822 (b5f3786) into master (7d1c50f) will increase coverage by 0.08%.
The diff coverage is 97.94%.

@@            Coverage Diff             @@
##           master    #2822      +/-   ##
==========================================
+ Coverage   97.96%   98.04%   +0.08%     
==========================================
  Files          67       68       +1     
  Lines       21800    21991     +191     
==========================================
+ Hits        21356    21561     +205     
+ Misses        444      430      -14     
Impacted Files Coverage Δ
seaborn/tests/_marks/test_bars.py 87.30% <ø> (+6.12%) ⬆️
seaborn/_core/plot.py 94.54% <54.54%> (-0.60%) ⬇️
seaborn/_marks/base.py 97.58% <100.00%> (+0.85%) ⬆️
seaborn/_marks/lines.py 100.00% <100.00%> (ø)
seaborn/objects.py 100.00% <100.00%> (ø)
seaborn/tests/_marks/test_lines.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d1c50f...b5f3786. Read the comment docs.

@mwaskom mwaskom merged commit fefd940 into master May 30, 2022
@mwaskom mwaskom deleted the nextgen/path branch May 30, 2022 00:45
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.

1 participant