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

Start Vizrank when opened, pause when closed #6614

Closed
wants to merge 8 commits into from

Conversation

janezd
Copy link
Contributor

@janezd janezd commented Oct 29, 2023

Issue

Fixes #6504. Also fixes #6624.

Description of changes
  • Vizrank starts immediately, as the user presses "Suggest Features", and closing it pauses it.
  • Re-opening after closing restarts it.
  • Changing the settings (number of attributes) also pauses it. The user can restart with the new setting or continue with the old.

(Rather) unrelated, I disabled duplicate-code issue in pylint, as we discussed a few weeks ago. This is a new test, it is too strict, and also unnecessary in our context: duplicated code was never an issue for us, so we will get just plenty of pointless issues. Including here, which is why I've had enough in this PR.

List of commits:

  • Commit 1: Auto-start/pause for Linear Projection.
  • Commit 2: Fixes Linear Projection: VizRank skips variables with missing data #6624.
  • Commit 3: Minor change in the runner. I think this is cleaner, plus it reduce the block enclosed in try-except .
  • Commit 4: Refactoring:
    • VizRankDialogNAttrs, a VizRank dialog in which user sets the number of variables, with functionality shared between Linear projection and Radviz
    • CloseVizrankMixin that should be mixed into all widgets with vizranks so that they stop vizrank when they are closed
    • auto_start: bool argument for VizRankDialog.add_vizrank, which causes computation to start when the vizrank button is pressed, and pauses it when dialog is closed.
  • Commit 5: Auto-start/pause for Radviz.
  • Commit 6: Auto-start/pause for Mosaic.
  • Commit 7: Sieve and Scatter plot.
  • Commit 8: Disable duplicate-code in pylintrc
Includes
  • Code changes
  • Some tests

@janezd janezd force-pushed the vizrank-start branch 8 times, most recently from 6f7dc91 to 3b3248c Compare November 4, 2023 09:35
@janezd janezd force-pushed the vizrank-start branch 3 times, most recently from 24c9490 to 501b847 Compare November 10, 2023 14:19
@janezd janezd changed the title [RFC] Linear Projection: Start Vizrank when opened, pause when closed Linear Projection, Radviz: Start Vizrank when opened, pause when closed Nov 10, 2023
Copy link

codecov bot commented Nov 10, 2023

Codecov Report

Merging #6614 (f614710) into master (ef5f426) will decrease coverage by 0.02%.
Report is 9 commits behind head on master.
The diff coverage is 82.97%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6614      +/-   ##
==========================================
- Coverage   88.10%   88.09%   -0.02%     
==========================================
  Files         321      321              
  Lines       69876    69956      +80     
==========================================
+ Hits        61563    61625      +62     
- Misses       8313     8331      +18     

@janezd
Copy link
Contributor Author

janezd commented Nov 10, 2023

Despite our discussion in the morning, I've put everything in the same PR. Linear projection and Radviz share most of the code. Mosaic would be based on this PR (and have similar code to the first two), which is annoying to maintain. Changes in Sieve and Scatterplot are trivial (when based on the code from this PR).

@janezd janezd changed the title Linear Projection, Radviz: Start Vizrank when opened, pause when closed Start Vizrank when opened, pause when closed Nov 17, 2023
@VesnaT
Copy link
Contributor

VesnaT commented Nov 17, 2023

When I restart Radviz Vizrank for a different Number of variables I get progress > 100%.

image

@VesnaT
Copy link
Contributor

VesnaT commented Nov 17, 2023

Scatter Plot
Reopening the Vizrank dialog reruns the procedure and causes duplicate rows:

image

Sieve Diagram
Reopening the Vizrank dialog reruns the procedure and causes duplicate rows:

image

@VesnaT
Copy link
Contributor

VesnaT commented Nov 17, 2023

Some observations:

  1. All Vizranks (for all widgets) start to recalculate when the dialog reopens, even if one only wants to select a different option.

  2. I also managed to get some errors that I cannot reproduce:

---------------------------- ValueError Exception -----------------------------
Traceback (most recent call last):
  File "/Users/vesna/orange-canvas-core/orangecanvas/scheme/signalmanager.py", line 1180, in __process_next
    if self.__process_next_helper(use_max_active=True):
  File "/Users/vesna/orange-canvas-core/orangecanvas/scheme/signalmanager.py", line 1218, in __process_next_helper
    self.process_node(selected_node)
  File "/Users/vesna/orange-canvas-core/orangecanvas/scheme/signalmanager.py", line 846, in process_node
    self.send_to_node(node, signals_in)
  File "/Users/vesna/orange-widget-base/orangewidget/workflow/widgetsscheme.py", line 806, in send_to_node
    self.process_signals_for_widget(node, widget, signals)
  File "/Users/vesna/orange-widget-base/orangewidget/workflow/widgetsscheme.py", line 820, in process_signals_for_widget
    process_signals_for_widget(widget, signals, workflow)
  File "/Users/vesna/miniconda3/envs/envorange/lib/python3.10/functools.py", line 889, in wrapper
    return dispatch(args[0].__class__)(*args, **kw)
  File "/Users/vesna/orange-widget-base/orangewidget/workflow/widgetsscheme.py", line 923, in process_signals_for_widget
    process_signal_input(input_meta, widget, signal, workflow)
  File "/Users/vesna/miniconda3/envs/envorange/lib/python3.10/functools.py", line 889, in wrapper
    return dispatch(args[0].__class__)(*args, **kw)
  File "/Users/vesna/orange-widget-base/orangewidget/workflow/widgetsscheme.py", line 886, in process_signal_input_default
    notify_input_helper(
  File "/Users/vesna/miniconda3/envs/envorange/lib/python3.10/functools.py", line 889, in wrapper
    return dispatch(args[0].__class__)(*args, **kw)
  File "/Users/vesna/orange-widget-base/orangewidget/utils/signals.py", line 735, in set_input_helper
    handler(*args)
  File "/Users/vesna/orange-widget-base/orangewidget/utils/signals.py", line 208, in summarize_wrapper
    method(widget, value)
  File "/Users/vesna/orange3-prototypes/orangecontrib/prototypes/widgets/owinteractions.py", line 340, in set_data
    self.apply()
  File "/Users/vesna/orange3-prototypes/orangecontrib/prototypes/widgets/owinteractions.py", line 344, in apply
    self.vizrank.initialize()
  File "/Users/vesna/orange3-prototypes/orangecontrib/prototypes/widgets/owinteractions.py", line 220, in initialize
    self.sel_feature_index = self.master.feature and data.domain.index(self.master.feature)
  File "/Users/vesna/orange3/Orange/data/domain.py", line 368, in index
    raise ValueError("'%s' is not in domain" % var)
ValueError: 'age' is not in domain
-------------------------------------------------------------------------------
---------------------------- ValueError Exception -----------------------------
Traceback (most recent call last):
  File "/Users/vesna/orange3/Orange/widgets/visualize/utils/__init__.py", line 337, in on_partial_result
    self._update()
  File "/Users/vesna/orange3/Orange/widgets/visualize/utils/__init__.py", line 357, in _update
    self._update_progress()
  File "/Users/vesna/orange3/Orange/widgets/visualize/utils/__init__.py", line 360, in _update_progress
    self.progressBarSet(int(self.saved_progress * 100 / max(1, self.state_count())))
  File "/Users/vesna/orange3/Orange/widgets/visualize/owlinearprojection.py", line 54, in state_count
    return factorial(n_all_attrs) // (2 * factorial(n_all_attrs - n_attrs) * n_attrs)
ValueError: factorial() not defined for negative values
-------------------------------------------------------------------------------

--------------------------- StopIteration Exception ---------------------------
Traceback (most recent call last):
  File "/Users/vesna/orange3/Orange/widgets/utils/concurrent.py", line 547, in _on_task_done
    self.on_exception(ex)
  File "/Users/vesna/orange3/Orange/widgets/utils/concurrent.py", line 492, in on_exception
    raise ex
  File "/Users/vesna/miniconda3/envs/envorange/lib/python3.10/concurrent/futures/thread.py", line 58, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/Users/vesna/orange3/Orange/widgets/visualize/utils/__init__.py", line 440, in run_vizrank
    next_state = next(states)
StopIteration
-------------------------------------------------------------------------------

  1. Beside that, after some testing (opening/reopening Vizrank dialogs for several widgets and changing the input file) the Orange got unresponsive and I had to kill it. It looks like something is calculating in the background, eventhough all the windows are closed.

@VesnaT VesnaT self-requested a review November 17, 2023 11:01
@VesnaT
Copy link
Contributor

VesnaT commented Nov 17, 2023

  1. File (HD) -> Linear Projection -> Suggest Features (leave it open)
  2. Disconnect the File
  3. Linear Projection (change the Number of variables) -> Restart with 5 variables

-------------------------- AttributeError Exception ---------------------------
Traceback (most recent call last):
  File "/Users/vesna/orange3/Orange/widgets/utils/concurrent.py", line 547, in _on_task_done
    self.on_exception(ex)
  File "/Users/vesna/orange3/Orange/widgets/utils/concurrent.py", line 492, in on_exception
    raise ex
  File "/Users/vesna/miniconda3/envs/envorange/lib/python3.10/concurrent/futures/thread.py", line 58, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/Users/vesna/orange3/Orange/widgets/visualize/utils/__init__.py", line 440, in run_vizrank
    next_state = next(states)
  File "/Users/vesna/orange3/Orange/widgets/visualize/owlinearprojection.py", line 58, in iterate_states
    self.attrs = self._score_heuristic()
  File "/Users/vesna/orange3/Orange/widgets/visualize/owlinearprojection.py", line 108, in _score_heuristic
    domain = self.master.data.domain
AttributeError: 'NoneType' object has no attribute 'domain'
-------------------------------------------------------------------------------

@VesnaT
Copy link
Contributor

VesnaT commented Nov 17, 2023

I suppose I got 0 number of variables when changing input to titanic.
image

@VesnaT
Copy link
Contributor

VesnaT commented Nov 17, 2023

This will never finish

image

@janezd
Copy link
Contributor Author

janezd commented Nov 17, 2023

Scatter Plot Reopening the Vizrank dialog reruns the procedure and causes duplicate rows:
Sieve Diagram Reopening the Vizrank dialog reruns the procedure and causes duplicate rows:

These two had the same cause. Fixed, I hope.

@janezd
Copy link
Contributor Author

janezd commented Nov 17, 2023

Vizrank used to be handled by separate (but repetitive) code in each widget. The current common vizrank classes were created in 2018 by a programmer who was inexperienced and young, not even 50 years old. Today, I'd do it differently, and definitely better. But wait for me when I'm 60!

Most problems in this PR results from design that is too rigid to be changed, or was even broken, but the errors were masked, e.g.

  1. File (HD) -> Linear Projection -> Suggest Features (leave it open)
  2. Disconnect the File
  3. Linear Projection (change the Number of variables) -> Restart with 5 variables

On master, disconnecting the input doesn't clear the vizrank, so the user can still select combinations. This is wrong, but we handle this by showing an error indicator saying "An error occurred while projecting data". The proper solution would be to call vizrank.initialize() even when there is no data, but @VesnaT :) enclosed this into an if that checks for data; this was needed because (unlike initialize in scatter plot, which empties the dialog when it has no data) initialize in linear projection crashes when master widget has no data.

@janezd
Copy link
Contributor Author

janezd commented Nov 18, 2023

When I restart Radviz Vizrank for a different Number of variables I get progress > 100%.

The particular reason for this was that I forgot to remove an unnecessary call to setProgressBar, so I think it counted combinations or something. But one can still achieve >100% progress by just pausing and restarting. This is unrelated to PR and comes from two sources:

I'm writing this just to show how fragile is this code. I'll try to reimplement the part that runs vizrank (that is, the thread), but fear that I'll end up writing a new, alternative set of classes for vizrank.

@janezd janezd self-assigned this Nov 24, 2023
@janezd janezd mentioned this pull request Dec 1, 2023
3 tasks
@janezd
Copy link
Contributor Author

janezd commented Dec 2, 2023

Closed in favor of #6661.

@janezd janezd closed this Dec 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants