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: Handle subset data #3620

Merged
merged 1 commit into from
Feb 22, 2019
Merged

Conversation

VesnaT
Copy link
Contributor

@VesnaT VesnaT commented Feb 21, 2019

Issue

Fixes #3617

Includes
  • Code changes
  • Tests
  • Documentation

@codecov
Copy link

codecov bot commented Feb 21, 2019

Codecov Report

Merging #3620 into master will decrease coverage by <.01%.
The diff coverage is 88.46%.

@@            Coverage Diff             @@
##           master    #3620      +/-   ##
==========================================
- Coverage   84.07%   84.07%   -0.01%     
==========================================
  Files         370      370              
  Lines       67256    67267      +11     
==========================================
+ Hits        56546    56553       +7     
- Misses      10710    10714       +4

@janezd janezd self-assigned this Feb 21, 2019
@janezd
Copy link
Contributor

janezd commented Feb 21, 2019

I agree with extracting _handle_subset_data to reduce the complexity of handleNewSignals.

But I'm not too happy with the fix. The root cause of the bug is that handleNewSignals does not call super().handleNewSignals(). We'll get similar bugs every time somebody adds something to OWDataProjectionWidget.handleNewSignals (or OWDataProjectionWidget.setup_plot(): OWDataProjectionWidget.handleNewSignals calls it and OWMDS.handleNewSignals doesn't!).

Derived methods that don't call inherited methods are a violation of OOP principles and a potential source of problems (especially when unexpected) that we should avoid. (Please don't say look, who's preaching! :))

You can:

  • change OWMDS.handleNewSignals to call the inherited method, possibly moving some code from OWMDS.handleNewSignals to OWMDS.setup_plot, or
  • remove OWMDS.handleNewSignals entirely and move all of its (additional) code to OWMDS.setup_plot (I would prefer this solution because this code indeed seems to belong there),
  • convince me that above solutions are too impractical.

@VesnaT VesnaT force-pushed the fix_mds_subset_data branch from 8dbf28e to 2d9ea06 Compare February 22, 2019 06:56
@VesnaT VesnaT changed the title MDS: Handle subset data [FIX] MDS: Handle subset data Feb 22, 2019
@janezd janezd merged commit a05809c into biolab:master Feb 22, 2019
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.

2 participants