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

Adding SimpleRidgeRegressor #129

Merged
merged 5 commits into from
May 15, 2019
Merged

Adding SimpleRidgeRegressor #129

merged 5 commits into from
May 15, 2019

Conversation

ayush-1506
Copy link
Collaborator

This is the first part in a series of fixes for #125.
Changes include:

  1. SimpleRidgeRegressor implementation.
  2. Modification of tests and docs.

@ablaom Since this is my first PR, I'd love a review.

@codecov-io
Copy link

codecov-io commented May 14, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@3d3c4f9). Click here to learn what that means.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #129   +/-   ##
=========================================
  Coverage          ?   67.57%           
=========================================
  Files             ?       19           
  Lines             ?     1252           
  Branches          ?        0           
=========================================
  Hits              ?      846           
  Misses            ?      406           
  Partials          ?        0
Impacted Files Coverage Δ
src/MLJ.jl 100% <ø> (ø)
src/builtins/ridge.jl 66.66% <66.66%> (ø)

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 3d3c4f9...9e18a91. Read the comment docs.

@giordano
Copy link
Member

giordano commented May 14, 2019

@ablaom if you don't want coveralls to create a comment in each PR, you can disable this feature in the settings: https://coveralls.io/github/alan-turing-institute/MLJ.jl/settings

Edit: just realised that the comment above is by codecov, which is more annoying to mute

@ablaom
Copy link
Member

ablaom commented May 14, 2019

Excellent, thanks. Some minor points:

  • in the test you should be giving fit a table not a matrix. The input X is always a table (unless input_is_multivariate(ModelType) = false). So just put ip = MLJ.table(ip') for the test.

  • make the name of the test file the same as the file being tested: test/SimpleRidgeRegressor -> test/ridge.jl

@ablaom ablaom merged commit 9e18a91 into JuliaAI:master May 15, 2019
@ablaom
Copy link
Member

ablaom commented May 15, 2019

Thanks for that!

There were some changes to LocalMultivariateStats.jl in tests that we didn't want, which I fixed.

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.

None yet

4 participants