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

bplotter.add_artist requires instance and type of instance? #22

Open
jo-mueller opened this issue Jul 18, 2024 · 1 comment
Open

bplotter.add_artist requires instance and type of instance? #22

jo-mueller opened this issue Jul 18, 2024 · 1 comment

Comments

@jo-mueller
Copy link
Contributor

Hi @zoccoler ,

I noticed that the API of the plotter.add_artist requires the type and and instance of the plot artist to be added:

    def add_artist(self, artist_type: ArtistType, artist_instance: Scatter | Histogram2D, visible: bool = False):
        """
        Adds a new artist instance to the artists dictionary.

        Parameters
        ----------
        artist_type : ArtistType
            The type of the artist, defined by the ArtistType enum.
        artist_instance : Scatter or Histogram2D
            An instance of the artist class.
        """

Wouldn't it be easier for the API to just pass the instance and derive the type inside the function? like, so:

    def add_artist(self, artist_instance: Scatter | Histogram2D, visible: bool = False):
        """
        Adds a new artist instance to the artists dictionary.

        Parameters
        ----------
        artist_instance : Scatter or Histogram2D
            An instance of the artist class.
        """

        artist_type = type(artist_instance)
@zoccoler
Copy link
Contributor

Hi @jo-mueller

That indeed would be simpler! Currently, I use ArtistType and SelectorType as keys to dictionaries (self.artists and self.selectors) whose values are the corresponding instances. Those dictionaries are used to check which artists or selectors were added to the CanvasWidget, but indeed the type does not seem necessary in the add_... method. If your implementation works, then it should also be applied to add_selector.

Could you send a PR? I would test and soon merge it.

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