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

[ENH] Add Groups column to Selected Data in Scatter plot output #2678

Merged
merged 3 commits into from
Oct 20, 2017

Conversation

janezd
Copy link
Contributor

@janezd janezd commented Oct 13, 2017

Issue

When selecting multiple groups in scatter plot, the Selected Data output contains no indication about the group.

Description of changes
  • Add a Groups column and corresponding tests
  • Refactoring of send_data.
Includes
  • Code changes
  • Tests
  • Documentation

self.Outputs.selected_data.send(selected)
self.Outputs.annotated_data.send(annotated)
data = self.data
domain = data.domain
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no check if there is data or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yet it survived the tests. Not good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, it didn't, I just didn't run all tests. Travis caught me. That's good.

Copy link
Contributor

@jerneju jerneju left a comment

Choose a reason for hiding this comment

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

See comment.

domain.metas + (group_var, ))
metas = np.hstack((self.data.metas[selection], groups - 1))
return Table(
sel_domain, data.X[selection], data.Y[selection], metas)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering, could you somehow reuse/extend create_annotated_table() for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started by filtering the annotated data, but it was messy. Now I've changed the create_groups_table and it's way simpler. Thanks for the suggestion.

@janezd janezd force-pushed the scatterplot-selection-groups branch 2 times, most recently from ca1a0bd to a56c18d Compare October 13, 2017 16:29

cls.signal_name = "Data"
cls.signal_data = cls.data

def setUp(self):
self.widget = self.create_widget(OWScatterPlot)

def _compare_selected_annotated_domains(self, selected, annotated):
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This disables an inapplicable test from the base class.

Hierarchical clustering, which has a similar situation, replaces this with checking that the domains have same variables and that the annotation appears only in one domain ... but I don't see much sense in testing this here, so I just disabled the test.

I now added a short comment explaining why the method is overridden.

Copy link
Contributor

Choose a reason for hiding this comment

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

I now added a short comment explaining why the method is overridden.

Thanks.

@codecov-io
Copy link

codecov-io commented Oct 13, 2017

Codecov Report

Merging #2678 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2678      +/-   ##
==========================================
+ Coverage   75.82%   75.82%   +<.01%     
==========================================
  Files         338      338              
  Lines       59459    59477      +18     
==========================================
+ Hits        45082    45101      +19     
+ Misses      14377    14376       -1

@janezd janezd force-pushed the scatterplot-selection-groups branch from a56c18d to 888bca5 Compare October 13, 2017 19:47
@janezd janezd force-pushed the scatterplot-selection-groups branch from 888bca5 to b3c0a0d Compare October 16, 2017 21:04
@janezd
Copy link
Contributor Author

janezd commented Oct 16, 2017

As suggested by @BlazZupan, the annotation column is now added as a class if the data had no class before. Any arguments against the change?

@janezd janezd added this to the 3.7 milestone Oct 17, 2017
@janezd janezd dismissed jerneju’s stale review October 18, 2017 19:54

The code in question was removed -- reimplemented differently.

return None
return create_groups_table(data, graph.selection, False, "Group")

def _get_annotated():
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's consider moving this somewhere else. Code in this function is used in many widgets.

@janezd
Copy link
Contributor Author

janezd commented Oct 20, 2017

You mean _get_annotated? It is useful only within send_data. We'd better consider if this send_data is common to all projection widgets (Scatter plot, MDS, Linear, FreeViz). It barely interact with the widget; maybe it belong to the graph -- not with this name, of course -- as a method that prepares the data sent by the widget. (I said maybe -- I haven't really looked into it.)

@jerneju jerneju merged commit ea87577 into biolab:master Oct 20, 2017
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