Implement interactive_errorbar() (and other fixes)#298
Implement interactive_errorbar() (and other fixes)#298JamesWrigley wants to merge 4 commits intompl-extensions:mainfrom
interactive_errorbar() (and other fixes)#298Conversation
This has been moved to a separate package: https://mpl-image-segmenter.readthedocs.io/en/latest/
JamesWrigley
force-pushed
the
callable
branch
from
8d72306 to
5bb61ba
Compare
for more information, see https://pre-commit.ci
|
Ok, CI passes now 🟢 |
|
(bump) |
|
sorry @JamesWrigley ! thank you for the bump. I will take a look today |
|
that was untrue I got caught up in things :( but today for sure |
|
No worries :) |
|
if only i hadn't gotten a real job |
ianhi
left a comment
ianhi
left a comment
There was a problem hiding this comment.
Really nice work @JamesWrigley
I have two blocking requests, each of which have some detail in the review on the lines.
- Can't remove image segmenter without importing the replacement and throwing up a strong warning
- simplify the callable check.
and a few other suggestions/thoughts that aren't hard requiremnts (see review).
Really glad to see errorbar here. it was a missing piece that definitely limited functionality. Curious what, if any, your motivating use case was?
| if not isinstance(arg, Callable): | ||
| return False | ||
| else: | ||
| return not hasattr(arg, "__array__") |
There was a problem hiding this comment.
I appreciate the docstring otherwise id be confused to find this down the line :)
Let's simplify this a bit into a one-liner (fine to keep as a function)
| if not isinstance(arg, Callable): | |
| return False | |
| else: | |
| return not hasattr(arg, "__array__") | |
| return isinstance(arg, Callable) and not hasattr(arg, "__array__") |
| container.remove() | ||
| container = ax.errorbar(x_, y_, yerr=yerr_, xerr=xerr_, fmt=fmt, **eb_kwargs) |
There was a problem hiding this comment.
This works. and I do think that that it a nice and important addition to the library. But I'd definitely prefer if we modified the data properties of the existing errorbar components instead of creating a new errorbar. This is more complex than this be has the benefits of improved performance (not constantly creating and throwing away complex artist objects) and crucially maintaining drawing order properly in complex plots.
We can use https://matplotlib.org/stable/api/container_api.html#matplotlib.container.ErrorbarContainer which is returned from the initial errorbar call
though that said the logic here is kinda a lot :( https://github.com/matplotlib/matplotlib/blob/v3.10.8/lib/matplotlib/axes/_axes.py#L3515-L3894
I wish that the logic of processing and then display logic was decoupled in mpl. that would make this so much easier.
So in conclusion would appreciate it if you took a crack at the more complex full update approach, but please don't sink too much time into it, this current state is still a definite improvement - but the limitations should be noted in the docstring.
|
|
||
| if not isinstance(xlim, str): | ||
| ax.set_xlim(xlim) | ||
| if not isinstance(ylim, str): | ||
| ax.set_ylim(ylim) | ||
|
|
||
| if hasattr(fig.canvas, "toolbar") and fig.canvas.toolbar is not None: | ||
| fig.canvas.toolbar.push_current() | ||
| sca(ax) |
There was a problem hiding this comment.
This is just a personal musing, not a request. Its a real mess that I have library with so much repeated code :( I should really have fixed that.
| class image_segmenter: | ||
| """Manually segment an image with the lasso selector.""" | ||
|
|
There was a problem hiding this comment.
We can't just remove this like this unfortunately. There are people out there who use this and I don't want to just break them. Instead please:
import from mpl-image-segmenter and make that avialable here with a compatibility layer accounting for the differences: https://mpl-image-segmenter.readthedocs.io/en/latest/differences.html
and raise a warning (unfortunately deprecation warnings get swallowed - so maybe a print statement, or a custom warning) that this going to go away and provide a link to the new package.
OR
don't delete, just delete whatever image_segmenter test was failing - and I will do the above in a follow up PR.
| "def x_func(tau):\n", | ||
| " return np.linspace(0, 2 * np.pi * tau, 50)\n", | ||
| "\n", | ||
| "\n", | ||
| "def y_func(x, tau):\n", | ||
| " return np.sin(x)\n", | ||
| "\n", | ||
| "\n", | ||
| "def yerr_func(tau):\n", | ||
| " return np.full(50, 0.1 * tau)\n", | ||
| "\n", | ||
| "\n", | ||
| "def xerr_func(tau):\n", | ||
| " return np.full(50, 0.05 * tau)\n", | ||
| "\n", | ||
| "\n", |
There was a problem hiding this comment.
I think this might be a better example if they weren't all linked via tau. feels little overloaded. Even if in real life use cases the period and the error are linked having a separate control for xerr and yerr and the values feels like it demonstrates the range of possiblities a bit
|
Also I was jsut looking at your profile and wanted to know more about what is going on at your xfel (I've been to the stanford one to do a beamtime once!) and noticed that your website link seems broken: https://jamesw.bio/ so heads up there |
2 participants
The bulk of the code implements
interactive_errorbar(), which allows passing x/y/xerr/yerr as functions. There are two other unrelated changes I made while writing this:image_segmenter, this was breaking the tests for me..__array__property, which makes the library play nicely with Julia (see MakeArrayValuenot callable JuliaPy/PythonCall.jl#728).Replaces #297.
Disclaimer: almost all of the changes were generated by Claude 🤖 Though I did go through it myself and make some tweaks.