intel / p3-analysis-library

A library simplifying the collection and interpretation of P3 data.
https://intel.github.io/p3-analysis-library/
MIT License
7 stars 10 forks source link

Move plot configuration arguments out of `kwargs` #46

Open laserkelvin opened 4 months ago

laserkelvin commented 4 months ago

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

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.

Pennycook commented 4 months ago

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)).