-
Notifications
You must be signed in to change notification settings - Fork 45
Implementation of matplotlib
backend for criterion_plot()
#599
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
matplotlib
backend for criterion_plot()
matplotlib
backend for criterion_plot()
556334a
to
d0c6856
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot! I have a few comments, if you have any questions let me know here or via Zulip. I think it will be important to get this PR right such that we don't complicate our lives when handling the other visualization functions.
My main concern is that I'd like the usage of the plotting class to feel more natural. See one of my comments in the backends module.
On an abstract level, a plotting class supporting the criterion plot needs to be able to:
- Create a figure
- Add lines to that figure
- Set a template
- Set a legend position
- Set axis labels
Now I would expect that our plotting class somehow incorporates / supports these actions or an action that combines a subset of them (e.g., if we think that some of these actions comprise one block, like we currently do with the PlotConfig).
Additionally
- As I am saying in one of the comments, we need to check whether we actually need the PlotConfig class.
- I thought about your proposal to rename
OptimizeResultOrPath
toResultOrPath
, and I think I prefer that! Could you change that?
Thanks a lot!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This looks very very good now for the first proper version.
I only have a few comments. Most importantly, we need to make sure that matplotlib
is treated as an optional dependency. Once this is handled we can move on to the other functions 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, thanks a lot! Only a few clarifying comments, and then we are hopefully through with this.
…n interactive mode.
95b665d
to
f15d982
Compare
Summary of Changes
backend
parameter tocriterion_plot()
which accepts "plotly" or "matplotlib"criterion_plot()
now returns the figure object corresponding to the chosen backend.backends.py
which serves as a registry for all backendsTo-Do
It is very much still a work in progress.
Plot