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] Widget Logistic Regression: can handle unused values #2116

Merged
merged 2 commits into from
Apr 7, 2017
Merged

[FIX] Widget Logistic Regression: can handle unused values #2116

merged 2 commits into from
Apr 7, 2017

Conversation

jerneju
Copy link
Contributor

@jerneju jerneju commented Mar 17, 2017

Issue

I can handle unused values
and instead of writing "coef" or sth similar I write a second value name
when only two values are considered.
https://sentry.io/biolab/orange3/issues/222366525/
https://sentry.io/biolab/orange3/issues/208659372/

Description of changes
Includes
  • Code changes
  • Tests
  • Documentation

@jerneju
Copy link
Contributor Author

jerneju commented Mar 17, 2017

@jerneju
Copy link
Contributor Author

jerneju commented Mar 17, 2017

data = Table("iris")
data = data[:80, :]
self.send_signal("Data", data)
self.widget.apply_button.button.click()
Copy link
Contributor

Choose a reason for hiding this comment

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

This only checks that the widget does not crash. Could you also test that the output is sensible?

Can you try it with keeping the first and the third class, or the second and the third?

@@ -92,10 +92,10 @@ def get_learner_parameters(self):
def create_coef_table(classifier):
i = classifier.intercept
c = classifier.coefficients
if len(classifier.domain.class_var.values) > 2:
values = classifier.domain.class_var.values
if c.shape[0] > 2:
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this something different? Wasn't the previous code correct? Isn't classifier.coefficients equal to the number of attributes + 1? Or am I overlooking something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previous code was probably not correct. c.shape[0] is not always equal to len(classifier.domain.class_var.values). That is in the case when we have a domain with more class values than contained in a data table. And of course always in the case when number of that values is 2.

@lanzagar lanzagar mentioned this pull request Mar 21, 2017
@astaric astaric added this to the 3.4.2 milestone Apr 7, 2017
@lanzagar
Copy link
Contributor

lanzagar commented Apr 7, 2017

Tests for this still do not pass. Have you looked into the issues caused by sgd reuse of the modified function?

I can handle unused values
and instead of writing "coef" or sth similar I write a second value name
when only two values are considered.
https://sentry.io/biolab/orange3/issues/222366525/
https://sentry.io/biolab/orange3/issues/208659372/
Test Logistic Regression: lint
@codecov-io
Copy link

Codecov Report

Merging #2116 into master will increase coverage by 0.97%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2116      +/-   ##
==========================================
+ Coverage   71.04%   72.02%   +0.97%     
==========================================
  Files         318      318              
  Lines       54563    54582      +19     
==========================================
+ Hits        38767    39314     +547     
+ Misses      15796    15268     -528

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45f282f...44267a1. Read the comment docs.

@lanzagar lanzagar merged commit 0858e4a into biolab:master Apr 7, 2017
@jerneju jerneju deleted the value-logisticregression branch April 20, 2017 13:47
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.

5 participants