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

[FIX] Fixes for deprecations in 3.26, and for changed behaviour of file dialog #4643

Merged
merged 4 commits into from
Apr 16, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 0 additions & 31 deletions Orange/widgets/data/contexthandlers.py

This file was deleted.

20 changes: 12 additions & 8 deletions Orange/widgets/data/owcorrelations.py
Original file line number Diff line number Diff line change
Expand Up @@ -248,9 +248,9 @@ class Outputs:

correlation_type: int

settings_version = 2
settings_version = 3
settingsHandler = DomainContextHandler()
selection = ContextSetting(())
selection = ContextSetting([])
feature = ContextSetting(None)
correlation_type = Setting(0)

Expand Down Expand Up @@ -307,7 +307,7 @@ def _feature_combo_changed(self):
self.apply()

def _vizrank_selection_changed(self, *args):
self.selection = [(var.name, vartype(var)) for var in args]
self.selection = list(args)
self.commit()

def _vizrank_stopped(self):
Expand All @@ -323,7 +323,7 @@ def _vizrank_select(self):
# filtered by a feature and therefore selection could not be found
selection_in_model = False
if self.selection:
sel_names = sorted(name for name, _ in self.selection)
sel_names = sorted(var.name for var in self.selection)
for i in range(model.rowCount()):
# pylint: disable=protected-access
names = sorted(x.name for x in model.data(
Expand All @@ -345,7 +345,7 @@ def set_data(self, data):
self.clear_messages()
self.data = data
self.cont_data = None
self.selection = ()
self.selection = []
if data is not None:
if len(data) < 2:
self.Warning.not_enough_inst()
Expand Down Expand Up @@ -415,7 +415,7 @@ def commit(self):

# data has been imputed; send original attributes
self.Outputs.features.send(AttributeList(
[self.data.domain[name] for name, _ in self.selection]))
[self.data.domain[var.name] for var in self.selection]))
Copy link
Contributor

Choose a reason for hiding this comment

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

self.data.domain[var.name] is a bit redundant, domain can be indexed with variables as well as names:
self.data.domain[var]

Copy link
Contributor

Choose a reason for hiding this comment

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

This should work even if var is loaded from settings and is not the same object right? Should have the same hash even in those exotic cases with compute_values etc? If not sure you can always ignore my comment above and leave it as it is now :)

Copy link
Contributor Author

@janezd janezd Apr 16, 2020

Choose a reason for hiding this comment

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

I thought so, too, but it doesn't work. compute_value is a part of a hash (and has to be, otherwise domain transformation would mistook two variables to be equivalent when they are not).

We have to use var.name here.

self.Outputs.correlations.send(corr_table)

def send_report(self):
Expand All @@ -426,8 +426,12 @@ def send_report(self):
def migrate_context(cls, context, version):
if version < 2:
sel = context.values["selection"]
context.values["selection"] = ([(var.name, vartype(var))
for var in sel[0]], sel[1])
context.values["selection"] = [(var.name, vartype(var))
for var in sel[0]]
if version < 3:
sel = context.values["selection"]
context.values["selection"] = ([(name, vtype + 100)
for name, vtype in sel], -3)
Comment on lines +429 to +434
Copy link
Contributor

Choose a reason for hiding this comment

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

I am confused, isn't selection now supposed to be a list of variables?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The widget's attribute (as in widget.selection) is a list of Variables. Context handler encodes it (as in context.values["selection"]) as a list of (name, type) pairs.

In settings version 2 the widget had an attribute selection which contains a list of (name, type) pairs. This was safe for saving, but does the encoding within the widget instead of in the handler. The handler thus saw strings with names of variables and issued a warning.

In settings version 3, the widget's selection is a list of Variables, and encoding is done within the handler.

Coincidentally, the encoded setting looks (almost) the same, because the widget's encoding was (almost) the same as encoding by the handler. So migration just adds 100 to vartypes and appends -3 to the tuple to mark it as a list.



if __name__ == "__main__": # pragma: no cover
Expand Down
135 changes: 80 additions & 55 deletions Orange/widgets/data/owselectcolumns.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,17 @@
)

from Orange.widgets import gui, widget
from Orange.widgets.data.contexthandlers import \
SelectAttributesDomainContextHandler
from Orange.widgets.settings import ContextSetting, Setting
from Orange.widgets.utils.listfilter import VariablesListItemView, slices, variables_filter
from Orange.widgets.settings import (
ContextSetting, Setting, DomainContextHandler
)
from Orange.widgets.utils import vartype
from Orange.widgets.utils.listfilter import (
VariablesListItemView, slices, variables_filter
)
from Orange.widgets.utils.widgetpreview import WidgetPreview
from Orange.widgets.utils.state_summary import format_summary_details
from Orange.widgets.widget import Input, Output, AttributeList, Msg
from Orange.data.table import Table
from Orange.widgets.utils import vartype
from Orange.widgets.utils.itemmodels import VariableListModel
import Orange

Expand Down Expand Up @@ -99,6 +101,45 @@ def dropMimeData(self, mime, action, row, column, parent):
return True


class SelectAttributesDomainContextHandler(DomainContextHandler):
def encode_setting(self, context, setting, value):
if setting.name == 'domain_role_hints':
value = {(var.name, vartype(var)): role_i
for var, role_i in value.items()}
return super().encode_setting(context, setting, value)

def decode_setting(self, setting, value, domain=None):
decoded = super().decode_setting(setting, value, domain)
if setting.name == 'domain_role_hints':
decoded = {domain[name]: role_i
for (name, _), role_i in decoded.items()}
return decoded

def match(self, context, domain, attrs, metas):
if not "domain_role_hints" in context.values:
return self.NO_MATCH

all_vars = attrs.copy()
all_vars.update(metas)
value = context.values["domain_role_hints"][0]
assigned = [desc for desc, (role, _) in value.items()
if role != "available"]
return assigned and \
sum(all_vars.get(attr) == vtype for attr, vtype in assigned) \
/ len(assigned)

def filter_value(self, setting, data, domain, attrs, metas):
if setting.name != "domain_role_hints":
return super().filter_value(setting, data, domain, attrs, metas)

all_vars = attrs.copy()
all_vars.update(metas)
value = data["domain_role_hints"][0].items()
data["domain_role_hints"] = {desc: role_i
for desc, role_i in value
if all_vars.get(desc[0]) == desc[1]}


class OWSelectAttributes(widget.OWWidget):
# pylint: disable=too-many-instance-attributes
name = "Select Columns"
Expand Down Expand Up @@ -305,68 +346,52 @@ def __used_attrs_changed(self):
def set_data(self, data=None):
self.update_domain_role_hints()
self.closeContext()
self.domain_role_hints = {}

self.data = data
if data is not None:
self.openContext(data)
all_vars = data.domain.variables + data.domain.metas

var_sig = lambda attr: (attr.name, vartype(attr))

domain_hints = {var_sig(attr): ("attribute", i)
for i, attr in enumerate(data.domain.attributes)}

domain_hints.update({var_sig(attr): ("meta", i)
for i, attr in enumerate(data.domain.metas)})

if data.domain.class_vars:
domain_hints.update(
{var_sig(attr): ("class", i)
for i, attr in enumerate(data.domain.class_vars)})

# update the hints from context settings
domain_hints.update(self.domain_role_hints)

attrs_for_role = lambda role: [
(domain_hints[var_sig(attr)][1], attr)
for attr in all_vars if domain_hints[var_sig(attr)][0] == role]

attributes = [
attr for place, attr in sorted(attrs_for_role("attribute"),
key=lambda a: a[0])]
classes = [
attr for place, attr in sorted(attrs_for_role("class"),
key=lambda a: a[0])]
metas = [
attr for place, attr in sorted(attrs_for_role("meta"),
key=lambda a: a[0])]
available = [
attr for place, attr in sorted(attrs_for_role("available"),
key=lambda a: a[0])]

self.used_attrs[:] = attributes
self.class_attrs[:] = classes
self.meta_attrs[:] = metas
self.available_attrs[:] = available
self.info.set_input_summary(len(data), format_summary_details(data))
else:
if data is None:
self.used_attrs[:] = []
self.class_attrs[:] = []
self.meta_attrs[:] = []
self.available_attrs[:] = []
self.info.set_input_summary(self.info.NoInput)
return

self.openContext(data)
all_vars = data.domain.variables + data.domain.metas

def attrs_for_role(role):
return [attr for _, attr in sorted(
(domain_hints[attr][1], attr)
for attr in all_vars if domain_hints[attr][0] == role)]

domain = data.domain
domain_hints = {}
domain_hints.update(self._hints_from_seq("attribute", domain.attributes))
domain_hints.update(self._hints_from_seq("meta", domain.metas))
domain_hints.update(self._hints_from_seq("class", domain.class_vars))
domain_hints.update(self.domain_role_hints)

self.used_attrs[:] = attrs_for_role("attribute")
self.class_attrs[:] = attrs_for_role("class")
self.meta_attrs[:] = attrs_for_role("meta")
self.available_attrs[:] = attrs_for_role("available")
self.info.set_input_summary(len(data), format_summary_details(data))

def update_domain_role_hints(self):
""" Update the domain hints to be stored in the widgets settings.
"""
hints_from_model = lambda role, model: [
((attr.name, vartype(attr)), (role, i))
for i, attr in enumerate(model)]
hints = dict(hints_from_model("available", self.available_attrs))
hints.update(hints_from_model("attribute", self.used_attrs))
hints.update(hints_from_model("class", self.class_attrs))
hints.update(hints_from_model("meta", self.meta_attrs))
hints = {}
hints.update(self._hints_from_seq("available", self.available_attrs))
hints.update(self._hints_from_seq("attribute", self.used_attrs))
hints.update(self._hints_from_seq("class", self.class_attrs))
hints.update(self._hints_from_seq("meta", self.meta_attrs))
self.domain_role_hints = hints

@staticmethod
def _hints_from_seq(role, model):
return [(attr, (role, i)) for i, attr in enumerate(model)]

@Inputs.features
def set_features(self, features):
self.features = features
Expand Down
11 changes: 11 additions & 0 deletions Orange/widgets/data/tests/test_owfile.py
Original file line number Diff line number Diff line change
Expand Up @@ -374,11 +374,22 @@ def test_read_format(self):
def open_iris_with_no_spec_format(_a, _b, _c, filters, _e):
return iris.__file__, filters.split(";;")[0]

"""
orangewidgets.utils.filedialogs.open_filename_dialog changed behaviour
in https://github.com/biolab/orange-widget-base/pull/53. Until it is
released, one of the following tests fails, so this test is disabled.
If you read this and orange-widget-base has already been released,
uncomment this code and remove the first test

with patch("AnyQt.QtWidgets.QFileDialog.getOpenFileName",
open_iris_with_no_spec_format):
self.widget.browse_file()

# Worked before
self.assertIsNone(self.widget.recent_paths[0].file_format)
# Works after
self.assertEqual(self.widget.recent_paths[0].file_format, "Orange.data.io.TabReader")
"""

def open_iris_with_tab(*_):
return iris.__file__, format_filter(TabReader)
Expand Down
50 changes: 17 additions & 33 deletions Orange/widgets/data/tests/test_owselectcolumns.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,13 @@
from AnyQt.QtGui import QDragEnterEvent

from Orange.data import Table, ContinuousVariable, DiscreteVariable, Domain
from Orange.widgets.data.contexthandlers import \
SelectAttributesDomainContextHandler
from Orange.widgets.settings import ContextSetting
from Orange.widgets.utils import vartype
from Orange.widgets.utils.state_summary import format_summary_details
from Orange.widgets.tests.base import WidgetTest
from Orange.widgets.data.owselectcolumns \
import OWSelectAttributes, VariablesListItemModel
import OWSelectAttributes, VariablesListItemModel, \
SelectAttributesDomainContextHandler
from Orange.widgets.data.owrank import OWRank
from Orange.widgets.widget import AttributeList

Expand Down Expand Up @@ -56,20 +55,21 @@ def test_open_context(self):

widget = SimpleWidget()
self.handler.initialize(widget)
self.handler.open_context(widget, self.args[0])
domain = self.args[0]
self.handler.open_context(widget, domain)
self.assertEqual(widget.domain_role_hints,
{('d1', Discrete): ('available', 0),
('d2', Discrete): ('meta', 0),
('c1', Continuous): ('attribute', 0),
('d3', Discrete): ('attribute', 1),
('d4', Discrete): ('attribute', 2),
('c2', Continuous): ('class', 0)})
{domain['d1']: ('available', 0),
domain['d2']: ('meta', 0),
domain['c1']: ('attribute', 0),
domain['d3']: ('attribute', 1),
domain['d4']: ('attribute', 2),
domain['c2']: ('class', 0)})

def test_open_context_with_imperfect_match(self):
self.handler.bind(SimpleWidget)
context1 = Mock(values=dict(
domain_role_hints=({('d1', Discrete): ('attribute', 0),
('m2', Discrete): ('meta', 0)})
('m2', Discrete): ('meta', 0)}, -2)
))
context = Mock(values=dict(
domain_role_hints=({('d1', Discrete): ('available', 0),
Expand All @@ -84,30 +84,14 @@ def test_open_context_with_imperfect_match(self):

widget = SimpleWidget()
self.handler.initialize(widget)
self.handler.open_context(widget, self.args[0])
domain = self.args[0]
self.handler.open_context(widget, domain)

self.assertEqual(widget.domain_role_hints,
{('d1', Discrete): ('available', 0),
('d2', Discrete): ('meta', 0),
('c1', Continuous): ('attribute', 0),
('c2', Continuous): ('class', 0)})

def test_open_context_with_no_match(self):
self.handler.bind(SimpleWidget)
context = Mock(values=dict(
domain_role_hints=({('d1', Discrete): ('available', 0),
('d2', Discrete): ('meta', 0),
('c1', Continuous): ('attribute', 0),
('d3', Discrete): ('attribute', 1),
('d4', Discrete): ('attribute', 2),
('c2', Continuous): ('class', 0)}, -2),
required=('g1', Continuous),
))
self.handler.global_contexts = [context]
widget = SimpleWidget()
self.handler.initialize(widget)
self.handler.open_context(widget, self.args[0])
self.assertEqual(widget.domain_role_hints, {})
{domain['d1']: ('available', 0),
domain['d2']: ('meta', 0),
domain['c1']: ('attribute', 0),
domain['c2']: ('class', 0)})


class TestModel(TestCase):
Expand Down
Loading