Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add TableOnHover and EmphasizeOnClick features #71

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

clara-moy
Copy link

I needed it for my work and i thought it could be useful for you too

@paulbrodersen
Copy link
Owner

paulbrodersen commented Jul 11, 2023

Hi, thanks for the pull request. On first reading, the code looks good and the writing style is very much in line with the rest of the code base, which I am very grateful for, as this makes my life very easy.

I am currently in the process of preparing a new major release. As part of that, I am re-evaluating how to handle some of the interactive features. For example, I am thinking about making annotations display on hover, rather than on click, much like "tooltips" in other libraries, and in line with your TableOnHover class. However, I don't want to support all possible combinations of emphasis, annotations, and tables with hovering or clicking. I am hence trying to figure out what are the most useful combinations and why. Could you expand a bit on your use-case and/or why you prefer to see tables on hover but emphasis on click? The latter surprised me.

@clara-moy
Copy link
Author

Hi,

In the network graph I want, each node represent a device that communicates with other devices. The communications are represented by the edges. The mapping I use is not the default mapping because communications sometimes have intermediate devices between the source and the destination. That is why I want to see which nodes communicated with the selected node, and then I want to see the info about those nodes without loosing the information of "who did the first node communicate with". In a nutshell, I want to emphasize on click and then hover emphasized nodes to see information

@paulbrodersen
Copy link
Owner

Hi @clara-moy, thanks for the quick response, that was very helpful, and I think that both classes are a good addition.

However, having looked at your code a little bit more in detail, I think there are a few issues that I would like addressed.
For example, I think that the new classes can be implemented more succinctly through inheritance. This may require a few minor changes in the existing classes. For example, TableOnHover, really should just be along the lines of:

class TableOnClick(object):
    def __init__(self, artist_to_table, table_kwargs=None):
        ...
        self._initialize_figure_callback()

    def _initialize_figure_callback(self):
        self.fig.canvas.mpl_connect("button_release_event", self._on_release)


class TableOnHover(TableOnClick):
    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)

    def _initialize_figure_callback(self):
        self.fig.canvas.mpl_connect("motion_event", self._on_release)

Unfortunately, I will be on vacation starting next week. If you want, I can talk you through the changes once I am back in a couple of weeks. Otherwise, I can implement these classes myself and list you as a co-author. For that, however, I would need to know your email address, either the one you are using for commiting in your own repos or your no-reply email address.

@clara-moy
Copy link
Author

Hi,

Thank you for your corrections. I will be on holidays too so I won't have the time to do it in the next few weeks. Moreover, I'm not sure I would be able to correct the code wisely (I'm only undergraduate). It seems that you have a good idea of how the code could be improved, so I think that it would be better if you did it. My email address is c.m0y@yahoo.com. Thank you for your help and I'm sorry I can't do it myself.

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

Successfully merging this pull request may close these issues.

2 participants