Skip to content

Fix calls to set_domain#5324

Merged
ajdapretnar merged 3 commits intobiolab:masterfrom
janezd:owmergedata-empty-table
Mar 29, 2021
Merged

Fix calls to set_domain#5324
ajdapretnar merged 3 commits intobiolab:masterfrom
janezd:owmergedata-empty-table

Conversation

@janezd
Copy link
Copy Markdown
Contributor

@janezd janezd commented Mar 10, 2021

Issue

Fixes #5323, and the same in three other widgets.

This happenned because empty table is also considered false, hence data and data.domain resulted in passing a Table (instead of None or an instance of Domain) when the table was empty.

I'd prefer not to write tests for this, though they'd be trivial. I don't like tests that just replicate a bug. Of course one could say "but you should test that a widget does not crash on empty table", but then we should add such a test to all widgets, not just the four that happenned to have this problem.

Includes
  • Code changes
  • Tests

@janezd janezd force-pushed the owmergedata-empty-table branch from 6038c97 to 3f7f34a Compare March 12, 2021 09:58
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 12, 2021

Codecov Report

Merging #5324 (0a809e4) into master (63ed27f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #5324   +/-   ##
=======================================
  Coverage   86.29%   86.30%           
=======================================
  Files         301      301           
  Lines       61191    61193    +2     
=======================================
+ Hits        52806    52810    +4     
+ Misses       8385     8383    -2     

@ajdapretnar
Copy link
Copy Markdown
Contributor

ajdapretnar commented Mar 26, 2021

I have a couple of issues, not necessarily with this fix, but let's discuss what needs to be solved where.

  1. Merge Data. The first issue is now solved, so when I connect from Pivot Table to Merge Data. But if I connect the second table to Merge Data, a new, but related issue appears.
Traceback (most recent call last):
  File "/Users/ajda/orange/orange-canvas-core/orangecanvas/scheme/signalmanager.py", line 1010, in __process_next
    if self.__process_next_helper(use_max_active=True):
  File "/Users/ajda/orange/orange-canvas-core/orangecanvas/scheme/signalmanager.py", line 1048, in __process_next_helper
    self.process_node(selected_node)
  File "/Users/ajda/orange/orange-canvas-core/orangecanvas/scheme/signalmanager.py", line 679, in process_node
    self.send_to_node(node, signals_in)
  File "/Users/ajda/opt/miniconda3/envs/o3/lib/python3.8/site-packages/orangewidget/workflow/widgetsscheme.py", line 792, in send_to_node
    self.process_signals_for_widget(node, widget, signals)
  File "/Users/ajda/opt/miniconda3/envs/o3/lib/python3.8/site-packages/orangewidget/workflow/widgetsscheme.py", line 833, in process_signals_for_widget
    widget.handleNewSignals()
  File "/Users/ajda/orange/orange3/Orange/widgets/data/owmergedata.py", line 369, in handleNewSignals
    self.openContext(self.data and self.data.domain,
  File "/Users/ajda/opt/miniconda3/envs/o3/lib/python3.8/site-packages/orangewidget/widget.py", line 1113, in openContext
    self.settingsHandler.open_context(self, *a)
  File "/Users/ajda/orange/orange3/Orange/widgets/data/owmergedata.py", line 191, in open_context
    self._encode_domain(domain1),
  File "/Users/ajda/orange/orange3/Orange/widgets/data/owmergedata.py", line 210, in _encode_domain
    all_vars = chain(domain.variables, domain.metas)
AttributeError: 'Table' object has no attribute 'variables'
  1. Correlations. Now it reports 0 instances on the input and a warning "At least two instances are needed". Transpose reports None (-). We should decide on one option, IMO.
  2. Create Class. Report None (-) and a warning "Data contains only numeric variables." Shouldn't widgets report "No data on input" or something?

@janezd janezd force-pushed the owmergedata-empty-table branch from 3f7f34a to 0a809e4 Compare March 27, 2021 11:55
@janezd
Copy link
Copy Markdown
Contributor Author

janezd commented Mar 27, 2021

  1. Fixed. (The general DomainContextHandler accepts Domain or Table; MergeDataContextHandler didn't, but now does.)
  2. Inconsistencies in summaries will be fixed by auto-summaries ([ENH] Automated and better summaries #5308).
  3. I fixed the warning.

As for whether the widget should report that the data table is empty: I wouldn't, because in this case every widget would have such a warning (or even an error) with the corresponding additional code ... I think a "0" in the summary should suffice, unless some particular widget needs to show a warning -- for instance a widget that need at least two instance and wants to inform the user that a single instance is not enough.

@ajdapretnar
Copy link
Copy Markdown
Contributor

Ok, let's go with a 0. It is just slightly confusing from a user's perspective, since we denote "empty" signals with a dashed line. Here, the line is full. But I agree, people should get used to using the status bar. ;)

@ajdapretnar ajdapretnar merged commit cf86692 into biolab:master Mar 29, 2021
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.

Merge Data: crash on empty data with domain

2 participants