-
-
Notifications
You must be signed in to change notification settings - Fork 411
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
Restructure plotting #1312
Restructure plotting #1312
Conversation
@ahartikainen Do you need help with this? Seems like theres a lot going on |
Sure if you have time. I will continue when I have some time. Basically there is couple of lines in each main plot function that is moved to backend functions, and then some of the inner kw are changed. Then there is still a lot to do. I will add discussion here when the time is right. |
51a8a0c
to
3f5f515
Compare
Codecov Report
@@ Coverage Diff @@
## master #1312 +/- ##
==========================================
- Coverage 93.00% 92.94% -0.06%
==========================================
Files 101 101
Lines 10115 10184 +69
==========================================
+ Hits 9407 9466 +59
- Misses 708 718 +10
Continue to review full report at Codecov.
|
Wow @ahartikainen you did it. Is this still in draft stage? I'm dying to hit that merge button |
It still in draft. There are some normalization to be done |
Ready for review. This PR enables mpl=>3.3 and doesn't change the user-interface. |
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 for all the work!
plot_ref_kwargs.setdefault("line_color", "black") | ||
plot_ref_kwargs.setdefault("line_dash", "dashed") | ||
else: | ||
plot_ref_kwargs.setdefault("alpha", 0.1) |
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.
not sure if we should use alpha
or line_alpha
, in fact, not even sure if we should create a bokeh dealiaser too
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.
bokeh kwargs are currently a mess, so I didn't touch them for now.
Also most bokeh kwargs are currently called explicitly
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.
Let's leave it like this then
@OriolAbril @canyon289 I'm starting to think that maybe we should move closer to "native" bokeh settings and colors. This would mean that we don't change "red" to mpl red but use bokeh red. We could have handling for special colors
|
bd169e9
to
25181ce
Compare
For base colors, both libraries use CSS colors so The conversion using mpl helpers has the advantage that then, in addition to CSS colors, all the following are also supported:
Note: |
Ok, if the colors are the same, I'm fine with the helper function. |
I'll take a look at this in next ~24 hours but dont let me block a merge either |
I dont want there to be a merge conflict so I'm going to merge this and keep reviewing. If stuff comes up I'll make an issue ticket |
Restructure plotting.
backend related defaults to backend code.