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] Linear regression: Fix Elastic net; Fix Auto-apply buttons #1601

Merged
merged 1 commit into from
Sep 30, 2016

Conversation

janezd
Copy link
Contributor

@janezd janezd commented Sep 23, 2016

  • Elastic net were mixed L1 and L2 in the opposite proportions(!).
  • The related slider used to be in the wrong part; while this was already fixed a few weeks ago, the code for constructing the slider was still in add_bottom_buttons.
  • The widget did not have the auto-apply button and would fail the base learner widget tests; this was cause by the redundant method add_bottom_buttons.
  • The widget did not have any tests (I assume that it's because it'd fail them)

I added a test to the base test class for learner widgets, which checks whether the class has unconditional_apply method. This method is be required for proper implementation of force-apply. If the widget intentionally removes it, it will have to override this test (or avoid being tested).

@codecov-io
Copy link

Current coverage is 88.67% (diff: 100%)

Merging #1601 into master will not change coverage

@@             master      #1601   diff @@
==========================================
  Files            78         78          
  Lines          8134       8134          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           7213       7213          
  Misses          921        921          
  Partials          0          0          

Sunburst

Powered by Codecov. Last update ebca371...0a22480

Copy link
Contributor

@BlazZupan BlazZupan left a comment

Choose a reason for hiding this comment

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

Checked everything, works as expected now.

@BlazZupan BlazZupan merged commit 809e955 into biolab:master Sep 30, 2016
@janezd janezd deleted the fix-lin-reg branch April 5, 2019 17:31
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.

3 participants