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

Compare everything with everything else #840

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

da-h
Copy link
Contributor

@da-h da-h commented Mar 7, 2022

Description

This PR enables to compare any pane with any other pane using the same title.
(See video below).

Motivation and Context

Visdom's compare feature is one of its selling points, but unfortunately is only supported by scatter plots.
There are issues asking for such a feature, i.e.

This PR tries to increase the usability of this feature by enabling to compare any two window contents that share the same title.
(This design decision is due to its previous behavior, where scatter plots were merged based on their title.)

In more detail:

  • Essentially, the compare_env from server.py has been rewritten completely, mostly falling back to the previous behavior in case of scatter plots.
  • This also solves a minor bug: when the first env in the compare-list did not contain a window a later env did contain, the window would not show up. The new behavior shows a window if any env contains it.

How Has This Been Tested?

Added a title to every plot in my demo.py-copy. However, I strongly think that this feature should be tested thoroughly before merging. ;)

Screenshots (if appropriate):

(The textbox says: "Actually, the new feature can change any content to any other content using the same title.")
record2

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • For JavaScript changes, I have re-generated the minified JavaScript code.

@da-h
Copy link
Contributor Author

da-h commented Mar 7, 2022

Two minor things I was not so sure about:

  • when does a pane need to include env['reload']-data? in this case it seems to work without it, but i feel i am missing something here. (see TODO part in this MR)
  • the state-change triggers sometimes only after a mouseclick somewhere else. it did see another feature in visdom have the same "bug". it is more likely that I used react incorrectly, but is this perhaps a known issue?

@JackUrb JackUrb self-requested a review March 8, 2022 17:14
Copy link
Contributor

@JackUrb JackUrb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HI @da-h - this is an interesting feature, but it's also a fairly complicated one to get right. I really like the eager approach here to get all of the pane types functioning and represented, though some notes on a design that could be considered:
It could be reasonable to create some kind of abstract ComparePane which would be fed data when in compare mode, and then the ComparePane can contain both specialized behavior (treating scatterplots in a merge way, perhaps images in side-by-side), alongside default behavior (either what you've implemented currently, or side-by-side as well). compare_envs in general is in need of a refactor, so if design changes are required for something like this I'm happy to work through them (and ensure everything is tested!). What are your thoughts here?

@da-h
Copy link
Contributor Author

da-h commented Mar 9, 2022

Thanks the feedback!

I too thought about a side-by-side feature as well, not only for images, but for arbitrary plots as well. I am yet unsure how to implement this in a fitting way: by including a global switch for side-by-side vs. tabbed view or by giving every Pane a button to switch between those two modes, what are the defaults, etc.

I am also unsure in which way to go here abstractly. I see two possibilities

  1. As of now, you are right, this feature should be a part of a specialized ComparePane that handles the content of choice, possibly merging data if possible. This would keep Pane more clean and more importantly, the new Component could use its own state to keep track of the current view. (Currently, the main-file remaps the data into the respective Panes.) The only downside, as far as I can tell, is that the order of windows would change after re-initializing all Panes as ComparePanes, as currently, the content gets replaced in already available instances. (Is my guess correct?)
  2. After thinking about this a bit longer I see another more intrusive suggestion to implement this feature into the project, but surely beyond the scope of this PR. The idea is to make the "Compare Mode" the default mode in which data is presented, thus making the Pane just a view for a selection of data. This would broaden visdom's "dashboard"-like appearance to enable the user to connect arbitrary plots & windows into a set of views that represent the user-selected data. In the default "view"/case, it would show the data just as visdom does now, namely show every plot in a seperate Pane. The new feature would, however, allow a user to connect any selection of plots to be represented in the same Pane. I see allowing the user to group & show exactly the information that is needed at the respective moment as an extension of the already existing functionality used right now by the current compare feature, which is currently only limited to group the same plot of different environments.

In any case or outcome of the discussion, I am happy to implement a concrete proposal that takes backward compatibility into account and resubmit the PR.

@da-h da-h changed the title Compare everything with everything else WIP: Compare everything with everything else Mar 21, 2022
@da-h da-h changed the title WIP: Compare everything with everything else Compare everything with everything else Mar 21, 2022
@da-h da-h marked this pull request as draft March 21, 2022 07:50
@JackUrb
Copy link
Contributor

JackUrb commented Mar 22, 2022

Sorry for the delays on the continued review here - this is a hard one to navigate, especially when considering things like how it interacts with some of the other quality features you're adding (like #837 or #846).

I think generally option (1) is the easier route, and that ultimately combining it with some of the other functionality we already have (like saved views) then it provides a similar type of capability that (2) would provide, without the headache of a substantial refactor.

I can outline a clearer sample architecture next week for what I imagine this would look like if you'd like to pursue that!

@da-h
Copy link
Contributor Author

da-h commented Mar 23, 2022

I think generally option (1) is the easier route, and that ultimately combining it with some of the other functionality we already have (like saved views) then it provides a similar type of capability that (2) would provide, without the headache of a substantial refactor.

This is true. Also, (1) could lead a more gradual route to (2) with the possibility to change the defaults later (i.e. allowing every Pane to be comparable or so).

I can outline a clearer sample architecture next week for what I imagine this would look like if you'd like to pursue that!

Sure, your suggestions are very welcome! ;)
Where i am particularly unsure in the context of react is how to enable each subclass of Pane to be comparable using the new ComparePane that extends Pane itself. Presumably you would have to create ComparePane as a wrapper for another Pane-Object, right? But how would the function compare_envs trigger the switch of the classes (Pane to ComparePane) ?

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

Successfully merging this pull request may close these issues.

2 participants