Skip to content

[ENH] New signals: Trees, Forest #2801

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

Merged
merged 3 commits into from
Nov 30, 2017
Merged

[ENH] New signals: Trees, Forest #2801

merged 3 commits into from
Nov 30, 2017

Conversation

jerneju
Copy link
Contributor

@jerneju jerneju commented Nov 28, 2017

Issue

Fixes #2797

Description of changes

Tree Viewer
Pythagorean Tree
Pythagorean Forest
Geo Map

Includes
  • Code changes
  • Tests
  • Documentation

@jerneju jerneju requested a review from nikicc November 28, 2017 09:51
@codecov-io
Copy link

codecov-io commented Nov 28, 2017

Codecov Report

Merging #2801 into master will increase coverage by <.01%.
The diff coverage is 98.31%.

@@            Coverage Diff             @@
##           master    #2801      +/-   ##
==========================================
+ Coverage   76.16%   76.17%   +<.01%     
==========================================
  Files         338      338              
  Lines       59879    59910      +31     
==========================================
+ Hits        45606    45635      +29     
- Misses      14273    14275       +2

@jerneju jerneju changed the title [ENH] New signals: Trees, Forest and Geo Map [ENH] New signals: Trees, Forest Nov 28, 2017
@nikicc nikicc added this to the 3.8 milestone Nov 29, 2017
Copy link
Contributor

@nikicc nikicc left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Though, I have one minor comment. IMHO
self.widget.Inputs.tree
is better, cleaner and not that much longer than

w = self.widget
w.Input.tree

But it might be just a personal preference.

@nikicc
Copy link
Contributor

nikicc commented Nov 29, 2017

@lanzagar I suggest we merge this, if you still allow merging before the release.

@lanzagar
Copy link
Contributor

I agree introducing another variable and line of code is often not worth it just to save a few characters in a couple of lines after that. But it is a matter of preference and not a big issue so I would prefer not to wait with merging until the last minutes before a release.

@lanzagar lanzagar merged commit 97e0e82 into biolab:master Nov 30, 2017
@jerneju jerneju deleted the new-signals-trees branch November 30, 2017 09:33
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