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

Enable customization of plots #13

Closed Pennycook closed 9 months ago

Pennycook commented 1 year ago

Related issues

Proposed changes


@swright87, GitHub won't let me request you as an official reviewer, but I'd like you to take a look at this to ensure that it addresses your comments in #10. Do the new examples show clearly enough how this should work? Are there any common operations (besides adjusting the axis limits) that you think we should showcase in the examples?

Pennycook commented 1 year ago

@laserkelvin: As we discussed offline, I've had a go at restructuring things here to more cleanly separate the plotting functions from the objects. I'd appreciate it if you could take a look.

If I understand your grand vision correctly, I think this could be considered a step towards that. We could eventually move all of the usages of matplotlib that currently sit inside cascade() to member functions of backend.matplotlib.CascadePlot.

Pennycook commented 11 months ago

@swright87, @laserkelvin - What do you think of the proposed changes in 2ae61172488e4711aad9684e4e63bfd0b7865b82..e0777fea00eb06cb76f293366a583ea8d8cbafec?

While working on our P3HPC paper, I realized that it was very difficult to get access to the legend object created by matplotlib, and it was almost impossible to modify the legend after matplotlib had drawn it. I ended up with lots of plots with multiple legends, which obviously wasn't what I wanted 😅 .

The nicest minimal change to the existing interface I could come up with for interacting with the legend(s) is the one proposed here, where the user can specify exactly what arguments they'd like to be passed through to matplotlib.

The only other way I can see us making this nicer for people is if we design a new interface that isn't so closely tied to matplotlib, and/or which doesn't try to do everything in one shot... For example, if we could find a way to separate the function which plots the cascade (p3.plot.cascade) from the function that plots the legend (p3.plot.legend?) we could more cleanly separate the arguments. But I'm not really sure how practical that is.

Anyway, I'd appreciate your thoughts!

laserkelvin commented 11 months ago

Based on similar kind of functionality in libraries like seaborn that wrap matplotlib, I think this is as close as we can get without placing too much burden on maintaining p3

Pennycook commented 11 months ago

Based on similar kind of functionality in libraries like seaborn that wrap matplotlib, I think this is as close as we can get without placing too much burden on maintaining p3

Right... I think this is the philosophical question we need to answer. Are we a library that wraps matplotlib, or a domain-specific library for P3 plots? The proposed Bokeh and TeX backends push us closer to the latter, in my opinion.

I'm going to start thinking a bit more about what it would look like if we hid matplotlib a bit more, so we have some other alternatives to evaluate.

swright87 commented 11 months ago

I think your commits look good and its a much neater way to control things than my previous attempt with legend positioning. I think in the past to achieve what you're suggesting I have just disabled legends in most plots and then edited a PDF to get a legend from one of the PDFs.

On the issue of what p3 is, without the plotting functionality its a library loading data and doing a bit of math; so I would err on the side of it being a domain specific library for P3 plots.

Pennycook commented 11 months ago

I think your commits look good and its a much neater way to control things than my previous attempt with legend positioning. I think in the past to achieve what you're suggesting I have just disabled legends in most plots and then edited a PDF to get a legend from one of the PDFs.

Ah, that's interesting. That suggests we need a reliable way to turn the legends off, too. My commits removed this functionality by mistake.

On the issue of what p3 is, without the plotting functionality its a library loading data and doing a bit of math; so I would err on the side of it being a domain specific library for P3 plots.

I'm inclined to agree.


Taking everything from the above into account, I think what we might need to do is introduce a few more classes to encapsulate some of the matplotlib functionality that is currently exposed. I've had a go at a rough sketch, below -- if everybody (@swright87, @laserkelvin) thinks it looks okay, I'll implement it.

Current

fig = plt.figure(figsize=(6, 5))
ax = p3.plot.cascade(df)
plt.savefig("cascade.png", bbox_inches="tight")

Proposed

plot = p3.plot.cascade(df, figsize=(6, 5))
plot.save("cascade.png")

We can choose to expose some common controls, and pass anything we don't understand (e.g. figsize) through to the backend (matplotlib in this case). The main difference here is that users wouldn't have to interact with matplotlib at all to produce simple plots.

Proposed + Customization

legend = p3.plot.Legend(ncols=4, loc="upper center")
style = p3.plot.Style(colors=[...], markers=[...], lw=1)
plot = p3.plot.cascade(df, platform_legend=legend, platform_style=style)
plot.save("cascade.png")

Again, we can support a mix of defined common controls (e.g. ncols, colors) and values passed through to the backend (e.g. loc, lw). This is basically what is already implemented in this PR, except that users would control the plots using specific objects instead of dictionaries. Using objects looks slightly cleaner to me, and is probably easier to document.

We're already giving certain styling options special treatment (e.g. plat_cmap, app_markers) so there's no real functional change here. We'd just be grouping everything related to styling a specific aspect of a plot into one object.

Proposed + TeX Backend

legend = p3.plot.Legend(nrows=4)
plot = p3.plot.cascade(df, platform_legend=legend, backend="pgfplots")
plot.save("cascade.tex")
swright87 commented 11 months ago

Proposed + Customization

legend = p3.plot.Legend(ncols=4, loc="upper center")
style = p3.plot.Style(colors=[...], markers=[...], lw=1)
plot = p3.plot.cascade(df, platform_legend=legend, platform_style=style)
plot.save("cascade.png")

I think this looks like a logical solution to me. I guess you'll need a platform_legend, app_legend, platform_style and app_style; but they perhaps only hold a dictionary of settings used when actually plotting, with some sanity checking in there.

I can't think of a good reason that wouldn't work for the pgfplots backend too...

laserkelvin commented 11 months ago

I agree, this interface looks clean and like @swright87 says, I can't see any reason why it wouldn't work for other potential and future backends.

Optionally I think we might need a way to validate things we pass to respective backends, but as long as they have a Python interface it should be relatively easy with inspect from the standard library. If it's not as straight forward as that, then I'd say we give up on validation.

Pennycook commented 11 months ago

Thanks, both. I'll get to implementing this.

I agree that validation of the kwargs is going to be tricky. I think for backend-specific options we might just have to make sure we catch any exceptions, then print the error for the user to act on.

Pennycook commented 11 months ago

@swright87, @laserkelvin: This is now ready for review (again). 😄

I updated the examples so that you can get a feel for what the new interface looks like.

It's now possible to write something like this:

# Generate a cascade plot with custom style options
legend = p3.plot.Legend(loc="center left", bbox_to_anchor=(0.91, 0.225), ncols=2)
pstyle = p3.plot.PlatformStyle(colors="GnBu")
astyle = p3.plot.ApplicationStyle(markers=["x", "s", "p", "h", "H", "v"])
cascade = p3.plot.cascade(df, size=(6, 5), platform_legend=legend, platform_style=pstyle, application_style=astyle)

...to produce: customized-cascade

laserkelvin commented 11 months ago

...to produce: customized-cascade

I'm a little obsessive, but the legend not perfectly aligned vertically bothers me LOL

Pennycook commented 11 months ago

I'm a little obsessive, but the legend not perfectly aligned vertically bothers me LOL

Oh, me too. The only way I've found to line things up reliably (so far) is to make a fake legend using annotations, which I wasn't happy with as a solution. I'll open an issue so we don't forget about it.

Pennycook commented 9 months ago

Happy with everything in this PR. Looks great. Will get to work fixing the other PR when I have time to work with these changes.

Great. Please let us know if there's anything we can do to help, or if you encounter any problems with the new interface.