Accept Stat and/or Move(s) as anonymous positional args in Plot.add #2948
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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), whereStatand/orMovewere optional. This was extended to allow multipleMoveoperations, by also allowing(Mark, Stat, List[Move]). I would also like to support multipleStatoperations (e.g.,BinthenAgg).But the distinction between
Stat/Moveis not especially meaningful. I think it can be roughly defined in terms ofMoveonly changing the values in the data, not the shape of the data, whereasStatis 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
Markand some number of transforms." Rather than allowing "aStat, or aMove, or a list ofStat(s)and/orMove(s)",transformsare accepted in a variable positional argument list. So the following calls are all now valid:For the time being, it would require substantial refactoring to actually treat
StatandMoveoperations 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 oneStat, 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:
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=ormove=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:
scalesto allMoveobjects so that their call method has the same signature asStatobjects. We'd eventually need to do this, and might as well not worry about backwards compatibility.