Open da-h opened 2 years ago
Two minor things I was not so sure about:
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)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
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?)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.
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!
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
) ?
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.
470
487
(Works even with image-sliders enabled).
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:
compare_env
fromserver.py
has been rewritten completely, mostly falling back to the previous behavior in case of scatter plots.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 anyenv
contains it.How Has This Been Tested?
Added a
title
to every plot in mydemo.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.")
Types of changes
Checklist: