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

Plotting utilities #523

Closed
DinoBektesevic opened this issue Mar 14, 2024 · 0 comments · Fixed by #597
Closed

Plotting utilities #523

DinoBektesevic opened this issue Mar 14, 2024 · 0 comments · Fixed by #597

Comments

@DinoBektesevic
Copy link
Member

I was not aware of before (#520 at least) that we had a "ResultsVisualizer" class in analysis utils. I was recently asked to add some plotting utilities that I used in a different internal notebook in #514

This is a bit messy, I should have just fixed ResultsVisualizer, but I made a mistake. Mainly, it's a class with all of its methods static. This isn't really a Pythonic way of doing things, if there's no state to be tracked it doesn't belong in a class. Modules of functions are just fine when appropriately named, f.e. if we named the file as result_visualizer then the functions would in other code read like result_visualizer.plot_single_stamp for example.

I also think that name is too long, just plotting is fine. So generally I propose:

  • make a plotting module
  • make static methods of ResultVisualizer just functions in that module
  • order the keyword arguments to be the same between ResultsVisualizer and plotting utils from Add initial not-that-generic plotting utilities. #514
  • Simplify any methods we can so that at best we have one plotting command in it, then we can use kwargs to pass the many many matplotlib arguments onward and leave the user with the control.
  • try to minimize the number of functions that actually create their own figure and axes, it'd be best if they all could just take a single axis in and spit a modified axis out. That way plotting utils can be composable for the user in many different scenarios.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants