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

Accept Stat and/or Move(s) as anonymous positional args in Plot.add #2948

Merged
merged 7 commits into from
Aug 13, 2022

Conversation

mwaskom
Copy link
Owner

@mwaskom mwaskom commented Aug 10, 2022

Small code change that has fairly large implications for how to think about the specification of a plot layer.

Previously, a layer was defined as a tuple of (Mark, Stat, Move), where Stat and/or Move were optional. This was extended to allow multiple Move operations, by also allowing (Mark, Stat, List[Move]). I would also like to support multiple Stat operations (e.g., Bin then Agg).

But the distinction between Stat/Move is not especially meaningful. I think it can be roughly defined in terms of Move only changing the values in the data, not the shape of the data, whereas Stat is allowed to change the shape. But both are fundamentally transforms that are applied to the data, in serial, before plotting.

So in the revised API, the distinction is collapsed, and a layer can be thought of simply in terms of "a Mark and some number of transforms." Rather than allowing "a Stat, or a Move, or a list of Stat(s) and/or Move(s)", transforms are accepted in a variable positional argument list. So the following calls are all now valid:

Plot().add(mark, stat)
Plot().add(mark, move)
Plot().add(mark, stat, move)
Plot().add(mark, stat, move, move)
Plot().add(mark, move, move)

For the time being, it would require substantial refactoring to actually treat Stat and Move operations the same and apply them at the same point in the internal data pipeline. So the API currently enforces some limitations: there can be at most one Stat, and it has to come first.

This is mostly backwards compatible with existing code based on the betas, with a few exceptions. The following no longer work:

Plot().add(mark, stat=stat)
Plot().add(mark, move=move)
Plot().add(mark, stat, [move, move])

I think that the new approach is superior: it's easier to swap a stat in/out if the move does not need an explicit keyword argument if it's missing, and juggling the multiple nested brackets required to pass a list of move objects is annoying. But there may be a little code churn if you've been playing with the beta, sorry. (The error message for cases where stat= or move= keywords were used is also a little confusing, you'll just get "ValueError: If using all scalar values, you must pass an index").

Needs before merging:

  • Update to objects interface demo
  • Potentially adding scales to all Move objects so that their call method has the same signature as Stat objects. We'd eventually need to do this, and might as well not worry about backwards compatibility.

@mwaskom mwaskom added this to the v0.12.0 milestone Aug 10, 2022
@codecov
Copy link

codecov bot commented Aug 10, 2022

Codecov Report

Merging #2948 (a63afa8) into master (ae9080d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2948   +/-   ##
=======================================
  Coverage   98.30%   98.31%           
=======================================
  Files          69       69           
  Lines       23004    23070   +66     
=======================================
+ Hits        22615    22681   +66     
  Misses        389      389           
Impacted Files Coverage Δ
seaborn/_core/moves.py 100.00% <100.00%> (ø)
seaborn/_core/plot.py 97.37% <100.00%> (+0.03%) ⬆️
tests/_core/test_moves.py 100.00% <100.00%> (ø)
tests/_core/test_plot.py 98.59% <100.00%> (+0.03%) ⬆️
tests/test_axisgrid.py 99.38% <0.00%> (+0.01%) ⬆️
seaborn/axisgrid.py 97.20% <0.00%> (+0.01%) ⬆️

@mwaskom mwaskom changed the title WIP:Accept Stat and/or Move(s) as anonymous positional args in Plot.add Accept Stat and/or Move(s) as anonymous positional args in Plot.add Aug 10, 2022
@mwaskom mwaskom added the api label Aug 10, 2022
@mwaskom mwaskom merged commit e9ad419 into master Aug 13, 2022
@mwaskom mwaskom deleted the plot/transforms branch August 13, 2022 20:38
qiemem added a commit to qiemem/seaborn that referenced this pull request Jul 22, 2023
Fixes the type in accordance with the intention of mwaskom#2948
mwaskom pushed a commit that referenced this pull request Jul 22, 2023
Fixes the type in accordance with the intention of #2948
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