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] Hierarchical clustering: Make annotation a context setting #1748

Merged
merged 2 commits into from
Nov 29, 2016

Conversation

janezd
Copy link
Contributor

@janezd janezd commented Nov 11, 2016

Alternative to #1739.

Issue

OWHierarchialClustering: Annotation drop down does not keep context settings.

Description of changes

Annotation index is replaced with a context setting that contains an attribute or a string. Context are used only if the data are distances between rows. Otherwise, separate settings for axis=0 and axis=1 are used.

Settings are not migrated: the previous settings didn't work anyway.

Includes
  • Code changes
  • Tests
    - [ ] Documentation

@codecov-io
Copy link

codecov-io commented Nov 11, 2016

Current coverage is 88.86% (diff: 100%)

Merging #1748 into master will not change coverage

@@             master      #1748   diff @@
==========================================
  Files            82         82          
  Lines          8896       8896          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           7905       7905          
  Misses          991        991          
  Partials          0          0          

Sunburst

Powered by Codecov. Last update bf61805...a877fe1

@VesnaT
Copy link
Contributor

VesnaT commented Nov 15, 2016

Values in the combo should be the same when placing a widget on canvas (disconected) and after disconnecting data from it.

@VesnaT
Copy link
Contributor

VesnaT commented Nov 15, 2016

I think selection in the combo, when data are distances between columns, should be somehow saved as well.

@janezd
Copy link
Contributor Author

janezd commented Nov 18, 2016

  • Empty widget should have Enumeration as default.
  • If domain has a class, use it as default, otherwise Names, otherwise Evaluation.

@astaric
Copy link
Member

astaric commented Nov 18, 2016

Would it be possible to add a tests that checks that the settings are stored and retrieved? Both, for row an attribute distances?

@janezd
Copy link
Contributor Author

janezd commented Nov 18, 2016

The widget already implemented the above rules we discussed with @VesnaT: class was chosen if it existed, and Enumeration was preferred over None. The latter didn't work (and an empty widget had None instead of Enumeration) because of the bug in gui.comboBox, which is fixed in #1764. Fetch my repo and cherry pick bb88aad if you want to test.

Widget now behaves like in #1739: it has context settings for matrices with attached data, and separate settings for axis=0 and axis=1.

I added tests.

(Note to self: ea30352 is the commit with the previous version.)

@janezd janezd changed the title Hierarchical clustering: Make annotation a context settings [FIX] Hierarchical clustering: Make annotation a context setting Nov 18, 2016
@astaric astaric added this to the 3.3.9 milestone Nov 25, 2016
@VesnaT
Copy link
Contributor

VesnaT commented Nov 28, 2016

Make a workflow: File (iris) -> Select columns -> Distances -> Hieararhical Clustering.
It works fine. Than use Select Columns to remove Target Variable from domain. After clicking "Send", Hieararhical Clustering crashes.

@janezd
Copy link
Contributor Author

janezd commented Nov 28, 2016

@VesnaT, thanks. Fixed (lines 997 and 998 in the revised commit) and tested (test_domain_loses_class).

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.

4 participants