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

Move plot configuration arguments out of kwargs #46

Closed
2 of 3 tasks
laserkelvin opened this issue Apr 26, 2024 · 1 comment · Fixed by #70
Closed
2 of 3 tasks

Move plot configuration arguments out of kwargs #46

laserkelvin opened this issue Apr 26, 2024 · 1 comment · Fixed by #70
Labels
enhancement New feature or request
Milestone

Comments

@laserkelvin
Copy link
Contributor

Feature/behavior summary

Looking at p3.plot.backend.matplotlib.CascadePlot's function signature, options to change plot styling (e.g. adding more markers in #45) are somewhat hidden as kwargs, which feels like a little unintuitive for users.

In most design patterns, kwargs is used to unpack into functions/classes being called within the function without needing to specify every single signature. The argument being made here is that things like application_style are actually commonly used and explicitly part of CascadePlot's usage/definition, and should be correspondingly explicitly exposed.

Request attributes

  • Would this be a refactor of existing code?
  • Does this proposal require new package dependencies?
  • Would this change break backwards compatibility?

Related issues

#42, #45

Solution description

Using the matplotlib CascadePlot as an example, I'm proposing to refactor the __init__ call; I'm using type hints to make it more obvious:

def __init__(
        self,
        df,
        eff=None,
        size=(6, 5),
        fig=None,
        axes=None,
        application_style: Mapping | ApplicationStyle | None = None,
        **kwargs,
    ):

   ....
   if isinstance(application_style, Mapping):
       application_style = ApplicationStyle(**application_style)
   if application_style is None:
       application_style = ApplicationStyle()

Additional notes

I've highlighted ApplicationStyle in this case because it feels like something a user will most likely want to configure. Legends and platform_style are somewhat less likely, and could be kept in kwargs but I guess for consistency could be moved into optional args as well.

@laserkelvin laserkelvin added the enhancement New feature or request label Apr 26, 2024
@Pennycook
Copy link
Contributor

I think I agree, but I'd like us to take a quick look at what matplotlib does before we make a decision. The original decision to put all of this stuff in kwargs was driven by a desire to look and feel matplotlib-like, but we might have misinterpreted their design.

As an example, look at matplotlib.pyplot.bar. Most of the styling options there are not part of the interface. But matplotlib does document some of them as "Other parameters" and some of them as "kwargs"... It's not actually clear to me how it works, but I think "color" ends up in kwargs there.

Is there any conceptual difference here, or do you also find matplotlib unintuitive? Is the difference that (most of) the kwargs in bar are being forwarded to Rectangle? Is matplotlib just weird?


As an aside, I think I'd prefer:

def __init__(
        self,
        df,
        eff=None,
        *,
        size=(6, 5),
        fig=None,
        axes=None,
        application_style: Mapping | ApplicationStyle | None = None,
        **kwargs,
    ):

...because plot.cascade(df, "app") is a reasonable shorthand, but it becomes harder to reason about what is going on as we add arguments. Something like plot.cascade(df, "app", (6, 5)) looks much worse to me than plot.cascade(df, "app", size=(6, 5)).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants