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] MDS: No optimization when subset data #2675

Merged
merged 3 commits into from
Oct 20, 2017
Merged

[FIX] MDS: No optimization when subset data #2675

merged 3 commits into from
Oct 20, 2017

Conversation

jerneju
Copy link
Contributor

@jerneju jerneju commented Oct 12, 2017

Issue

1. MDS cannot handle sparse data. That will be fixed in #2690 .
2. Optimization runs when subset data is sent to the widget. This is not necessary.
3. Plot resets when Show similar pairs slider is moved.
4. Similar pairs are not removed when Show similar pairs slider is set to 0. And they should be removed.

Description of changes
Includes
  • Code changes
  • Tests
  • Documentation

@codecov-io
Copy link

codecov-io commented Oct 12, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@aabd844). Click here to learn what that means.
The diff coverage is 68.75%.

@@            Coverage Diff            @@
##             master    #2675   +/-   ##
=========================================
  Coverage          ?   75.81%           
=========================================
  Files             ?      338           
  Lines             ?    59497           
  Branches          ?        0           
=========================================
  Hits              ?    45107           
  Misses            ?    14390           
  Partials          ?        0

@jerneju jerneju requested a review from lanzagar October 12, 2017 08:01
@jerneju jerneju changed the title [WIP][FIX] MDS: sparse support [FIX] MDS: sparse support Oct 12, 2017
@jerneju jerneju changed the title [FIX] MDS: sparse support [FIX] MDS: sparse support and no optimization when subset data Oct 13, 2017
@janezd
Copy link
Contributor

janezd commented Oct 13, 2017

Both commits seem OK to me if this is what we want to do with sparse data. @nikicc, @ajdapretnar please confirm.

@janezd
Copy link
Contributor

janezd commented Oct 13, 2017

Add tests for sparse data.

@ajdapretnar
Copy link
Contributor

ajdapretnar commented Oct 16, 2017

This now works!
But sending Data Subset to MDS does not do anything. I mean it does not hollow out the points as it should... Am I missing something?

nikicc
nikicc previously requested changes Oct 17, 2017
@@ -271,6 +271,8 @@ def update_regression_line(self):

def init_attr_values(self):
domain = self.data and len(self.data) and self.data.domain or None
if domain is not None and self.data.is_sparse():
domain = Domain(attributes=[], class_vars=domain.class_vars)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why only class variables here? What about metas? If this is only here with the purpose of removing sparse features, I would prefer if we use filter_visible method from Orange.data.domain that is used elsewhere (e.g. select rows).

I'm not really a fan of filter_visible, but let's keep the same approach everywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we should support filtering by filter_visible in the model so we got this fixed once and for all?

Copy link
Contributor

Choose a reason for hiding this comment

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

#2690 should probably solve this issue.


attributes = self.data.domain.attributes + (self.variable_x, self.variable_y) + \
primitive_metas
if self.data.is_sparse():
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need a different logic for sparse here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@janezd janezd added this to the 3.7 milestone Oct 17, 2017
@nikicc
Copy link
Contributor

nikicc commented Oct 17, 2017

Also, on hover doesn't show all data for sparse. Is there a special reason for this?

@jerneju jerneju changed the title [FIX] MDS: sparse support and no optimization when subset data [FIX] MDS: No optimization when subset data Oct 20, 2017
@nikicc nikicc dismissed their stale review October 20, 2017 13:07

Not relevant any more, the code in question was moved to #2693

@lanzagar lanzagar merged commit 99bb572 into biolab:master Oct 20, 2017
@jerneju jerneju deleted the mds-sparse branch October 20, 2017 13:08
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.

6 participants