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

OWNomogram: Add a new widget #1936

Merged
merged 4 commits into from
Jan 27, 2017
Merged

OWNomogram: Add a new widget #1936

merged 4 commits into from
Jan 27, 2017

Conversation

VesnaT
Copy link
Contributor

@VesnaT VesnaT commented Jan 20, 2017

Issue
Description of changes
Includes
  • Code changes
  • Tests
  • Documentation

@codecov-io
Copy link

codecov-io commented Jan 20, 2017

Current coverage is 89.51% (diff: 100%)

Merging #1936 into master will decrease coverage by 0.01%

@@             master      #1936   diff @@
==========================================
  Files            90         90          
  Lines          9180       9182     +2   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           8219       8219          
- Misses          961        963     +2   
  Partials          0          0          

Sunburst

Powered by Codecov. Last update f3ebda7...5f781ce

@VesnaT VesnaT changed the title OWNomogram: Add a new widget [WIP] OWNomogram: Add a new widget Jan 20, 2017
@VesnaT VesnaT changed the title [WIP] OWNomogram: Add a new widget OWNomogram: Add a new widget Jan 20, 2017
Copy link
Contributor

@lanzagar lanzagar left a comment

Choose a reason for hiding this comment

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

A couple of suggestions:

  1. Some difference in the font for Points/Total/Probabilities vs. feature names would be very welcome. I think just changing feature names to not be bold would already make the visualization easier and quicker to process.
  2. Log. Reg. models always use align left (setting disabled). And for Naive Bayes, center alignment will mostly be used. I suggest removing this setting completely, and just selecting this automatically based on the classifier.
  3. I suggest removing the option Variance in Sort by. Together with absolute importance they are a bit redundant, as the latter incorporates it to a degree anyway - a variable can't distinguish classes well without enough variance. Variance of a variable is also not readable from this visualization (unlike other measures) and should be investigated somewhere else (box plot?).
  4. A better default would probably be to show the top 10 most informative variables (absolute importance). All and unsorted is probably going to be changed very quickly and every time for data with tens of variables or more.
  5. I was not yet able to figure out what the Continuous features: option is used for? It is disabled for data with discrete and/or continuous features.
  6. Total and Probabilities appear as separate graphical elements. IMHO it would be better to either:
    a) Have them actually be separate. In this case Probabilities could be on a linear scale from 0 to 1. (points not aligned)
    b) Use one line and one point, with values for Total/Prob. shown above/below.
    c) Keep two separate scales, but make it more clear they are connected by a small vertical line going through both (aligned) points.
    With b) and c) users are less likely to look only at Prob. (while changing feature sliders) and be confused by some peculiarities (like the cutoff and a deformed scale). Adding <= and >= to the first and last value could also help.

@VesnaT
Copy link
Contributor Author

VesnaT commented Jan 24, 2017

@janezd Could you please confirm the above suggestions?

@VesnaT VesnaT force-pushed the nomogram branch 2 times, most recently from 80abbf7 to 61de7e1 Compare January 26, 2017 06:24
Copy link
Contributor

@janezd janezd left a comment

Choose a reason for hiding this comment

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

@VesnaT, great. Snapping to the closest value is a cherry on top.

@lanzagar, I do have a few more ideas ... but if you agree, we'd merge this now and improve later.

@lanzagar
Copy link
Contributor

We noticed some bugs with @VesnaT yesterday, which she began fixing. When she confirms that is finished I am in favour of merging, and doing improvements in new PRs afterwards.

@VesnaT VesnaT force-pushed the nomogram branch 4 times, most recently from c505f78 to 77abd43 Compare January 27, 2017 13:14
@lanzagar lanzagar merged commit 75de2c8 into biolab:master Jan 27, 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