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

GUI freezing #4

Open
vpicouet opened this issue May 6, 2020 · 12 comments
Open

GUI freezing #4

vpicouet opened this issue May 6, 2020 · 12 comments
Assignees

Comments

@vpicouet
Copy link

vpicouet commented May 6, 2020

Hi!
Very nice tool you implemented!
On my mac, python 3.6 it is mostly working all right.
But once every 2-3 times, the gui (sliders) freezes and I did not manage do understand the reason (no warning etc).
It might be when the plot leaves the x/y limits of the window but sometimes even if it stays within the limits.
Did you already experienced this problem?
Best

Vincent

@glentner
Copy link
Owner

glentner commented May 6, 2020

Hi @vpicouet.

At times I've seen something like this when the fitting routine goes wonky and the parameters get set outside the bounds of the slider widgets. Typically, clicking inside the slider bounds will snap it back, but I imagine a lot of this is to do with the specific matplotlib backend at play.

Would you be comfortable sharing a screenshot?

@vpicouet
Copy link
Author

Sorry to get back to you so late, I did not work on this for a while.
It seems that the problem appears quite often when the matplotlib window pops up. Fortunately, we can actually have the widgets to work again if we go on another window and come back to the auto_gui window... weird.
Capture d’écran, le 2020-07-28 à 12 11 43
Also, is there a way with your package to actually plot (additionally to the total function), the different contributions (the background and each gaussian)
I use this, but I would like to see each model of the composite model...

        gui = AutoGUI(CompositeModel(*Models, label='General fit'), [model_curve], bbox=[0.20, 0.07, 0.75, 0.17],
                      slider_options={'color': 'steelblue'}, data=(xdata_i, ydata_i))

I tried to add a list of the curves as second argument but when I touch the sliders they disappear

        for modeli in Models:
            a, = self.ax.plot(xsample, modeli(xsample), color='grey', label=None, linestyle='dotted')
            curves.append(a)

        gui = AutoGUI(CompositeModel(*Models, label='General fit'), [model_curve]+curves, bbox=[0.20, 0.07, 0.75, 0.17],
                      slider_options={'color': 'steelblue'}, data=(xdata_i, ydata_i))

Thanks for your job, it's a nice package!

@glentner glentner self-assigned this Jul 28, 2020
@glentner
Copy link
Owner

This looks awesome, I'm glad you were able to make good use of this feature. Sorry again about your issues with the widgets becoming unresponsive. I imagine this is indeed a matplotlib issue in some capacity, likely to do with the backend in use.

Regarding the individual component models, actually I use to have a version of the code that did just that! I don't think there's a trivial way to make this happen with the current library. If you dig into the code for this, you'll see that there's a step where during the update it extracts the xdata for each graph and passes it to the model to get new ydata for each and updates the plots. A CompositeModel has access to each of its underlying models; in some way, we could allow for that update method to additionally pass the xdata through each component and create shadow graphs. What I had done in the past was to show each component with a thin dotted line under the blended model and when you select the radio button for the component it alters that associated component to be thicker/bold.

@vpicouet
Copy link
Author

vpicouet commented Jul 28, 2020

Yes, well your package is quite nice!
I guess you speak about this method:

    def __update_graph(self) -> None:
        """Re-draw curves based on current slider values."""
        for graph in self.graphs:
            graph.set_ydata(self.model(graph.get_xdata()))
            graph.figure.canvas.draw_idle()

I might try to dig at some point because it can be interesting to have the different contributions!

@vpicouet
Copy link
Author

vpicouet commented Jul 28, 2020

A rough way to do it is change it by, with the graphs I gave is to change __update_graph by:

    def __update_graph(self) -> None:
        """Re-draw curves based on current slider values."""
        
        for i, graph in enumerate(self.graphs):
            if i==0:
                graph.set_ydata(self.model(graph.get_xdata()))
            if i>0:
                graph.set_ydata(self.models[i-1](graph.get_xdata()))
            graph.figure.canvas.draw_idle()

But it might lead to side effects as I do not really know your code.

@glentner
Copy link
Owner

glentner commented Jul 28, 2020

Indeed. Eventually, I need to find some time to create some comprehensive documentation on these things as I have with some of my other projects. The elegance (and the downside) of the way this is coded is that there can be an arbitrary number of these graphs on different axes (and even different figures). You as a user can supply the graph and it simply does the updates with the model parameters.

So the challenge here is the fact that we are not actually tracking what the graphs (Line2D objects) are connected to; there is no semantic information. The solution is to at some point have lines drawn for each component and then simply have them updated. So we need a dictionary associating the components to their Line2D instance.

@glentner
Copy link
Owner

So a nice way that I thought of to handle this is to keep the user supplied graphs as currently implemented. But if we're a CompositeModel, we have access to the Axes for each graph and underlying xdata; so we can automatically draw "subgraphs" (Line2D instances) for each model, for each graph.

    @graphs.setter
    def graphs(self, val: List[mpl.lines.Line2D]) -> None:
        """Assign curves to update when interacting with widgets."""
        if val is None:
            self.__graphs = self.__create_default_graphs()
        elif not hasattr(val, '__iter__') or not all(isinstance(v, mpl.lines.Line2D) for v in val):
            raise TypeError('{0}.graphs expects all to Line2D, given {1}.'
                            .format(self.__class__.__name__, val))
        else:
            self.__graphs = list(val)
        if isinstance(self.model, CompositeModel):
            self.__component_graphs = {}
            for graph in self.__graphs:
                xdata = graph.get_xdata()
                self.__component_graphs[graph] = []
                for model in self.models:
                    subgraph, = graph.axes.plot(xdata, model(xdata), color='orange')
                    self.__component_graphs[graph].append((model, subgraph))

The update method would then look something like this.

    def __update_graph(self) -> None:
        """Re-draw curves based on current slider values."""
        for graph in self.graphs:
            graph.set_ydata(self.model(graph.get_xdata()))
            if isinstance(self.model, CompositeModel):
                for model, subgraph in self.__component_graphs[graph]:
                    subgraph.set_ydata(model(subgraph.get_xdata()))
            graph.figure.canvas.draw_idle()

This works well. But now I remember what the challenge was. What to do if one of your component models is actually a "background"?

Screenshot 2020-07-28 14 05 12

All of the components work, but what I think people would expect is that the "non-background" components are drawn on top of the background. We'll need to add some kind of marker to track and understand that.

@glentner
Copy link
Owner

I think it's a good idea to add the notion of a group to the Model class. So a CompositeModel not only is a collection of member models, those members belong to a set of groups. The AutoGUI then can have a concept of a "background" model by asking if the model is composite and has a group called "background" with a single Model. Then the AutoGUI can distinguish on the basis of that sort of classification; background models are plotted normally as subgraphs and non-background models are plotted in super-position with the background model.

I also was able to tell the update function for when a radio button was selected to alter the formatting of the associated subgraph for the active_model.

Screenshot 2020-07-28 17 42 04

It's rough, the color of the subgraphs is hard-coded, and I don't have any unit tests for it, but it works.

@vpicouet
Copy link
Author

That's a nice update, I think it should stay optional as users might want not to use it sometimes (on crowded plots) but it's exactly what I was looking for!

@glentner
Copy link
Owner

The best I can think of at the moment is to have an optional argument in the constructor to the AutoGUI just as there is for the slider_options, to control the original style options for the subgraphs. And maybe a create_subgraphs=False as a default argument.

@glentner
Copy link
Owner

I should also point out that depending on your data and the complexity of the model you've created, including these subgraphs can be an impediment to the smoothness of the interface; it not only has to re-compute the model and update the canvas for each change of the slider, but also all of the component subgraphs.

@vpicouet
Copy link
Author

vpicouet commented Aug 3, 2020

That's a good point, for now I am using pretty basic models with low number of points so the create_subgraphs option is quite interesting.
Thanks for the implementation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants