Quasars / orange-spectroscopy

Other
51 stars 59 forks source link

Tests: TestOWSpectra.test_settings_color fails to restore ContextSetting (Orange master) #410

Closed stuart-cls closed 4 years ago

stuart-cls commented 4 years ago

Edit: It's not these two things, see below.

Original: Orange master has deprecated storing variables in settings as strings, orangecontrib.spectroscopy.tests.test_owspectra.test_settings_color sets the feature_color directly.

ISTM this is part of a bigger issue looming from https://github.com/biolab/orange3/pull/4158 where we should stop using comboBox from Orange.widget.gui and instead use the one from orange-widget-base but I'm not confident about the version soup on that transition.

stuart-cls commented 4 years ago

The specific failure is the feature_color ContextSetting is not restored when the Iris dataset is re-sent to the widget.

stuart-cls commented 4 years ago

This was introduced with https://github.com/biolab/orange3/pull/4421 specifically https://github.com/biolab/orange3/commit/b983e25212aa3c006c37089ef970688e3c01c369 and has nothing to do with settings-as-strings or gui.combobox transition.

If you set settingsHandler = DomainContextHandler(first_match=False) in OWSpectra() the tests pass and the previous behaviour is restored but I'm not sure that's actually the correct solution.

@markotoplak thoughts?

stuart-cls commented 4 years ago

Actually the bug is still there with first_match=False, just reversed (iris=coloured -> iris=None -> housing -> iris=coloured oops)

markotoplak commented 4 years ago

@stuart-cls, thank you for this diagnosis. I will try to fix it tomorrow. I would like to look at this myself because we changed how contexts work in Orange so that they are more intuitive, and this tests probably uses some assumptions that are not valid anymore.

stuart-cls commented 4 years ago

Thanks @markotoplak .

I notice the test passes but the actual behaviour in the GUI doesn't match the test. Is this on purpose?

i.e. if I run Quasar (orange3-master, orange-spec-master) with settings reset, and then do the things in the test (iris -> iris coloured -> housing -> iris) it doesn't successfully restore the setting.

markotoplak commented 4 years ago

Yes, it does not. We changed handling of contexts so that the last valid context is taken. Because in the housing we did not select any colors, this is going to become the last valid context, and it is valid because nothing is selected.

I am not sure if this is the best thing to do in such case, but it can save us from strange color changes when selecting columns. There is no perfect solution, this one is at least is more predictable. We changed this because biolab/orange3/issues/4415

Loading files should still work properly.

Try it for a while. If you notice that this behaviour bothers you we can switch it back by adding a parameter to the context.

stuart-cls commented 4 years ago

Okay, thanks for the explanation. Let's try it as you suggest - hopefully it's not one of those things where once you know about it you notice all the time :)

markotoplak commented 4 years ago

Certainly you can. All widgets work like this one now. :)