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

Add VotingChainladder (resolves #115) #116

Merged
merged 2 commits into from
Feb 2, 2021
Merged

Conversation

cbalona
Copy link
Contributor

@cbalona cbalona commented Feb 2, 2021

No description provided.

@cbalona
Copy link
Contributor Author

cbalona commented Feb 2, 2021

Tests seems to be failing due to an sklearn import error: ImportError: cannot import name 'delayed' from 'sklearn.utils.fixes' (/usr/share/miniconda/envs/cl_test/lib/python3.7/site-packages/sklearn/utils/fixes.py)

@jbogaardt
Copy link
Collaborator

This is an awesome PR! I wouldn't have been able to come up with something this good.

Seems to only need a resolution for:

  1. the ImportError` - is it a specific version of sklearn that we need to limit to?

Not needed for the PR, but we should also:
2. Add the class to to docs/modules/classes.rst TOC so its docstring shows up in the API docs
3. Perhaps take your unit test and add a plot to get it as a stand-alone 'Example' in the /examples folder for those that browse the docs by the example gallery
4. Add ensemble.rst to docs/modules/index.rst so the docs show up.
4. Add it to README.rst on the main repo page

@cbalona
Copy link
Contributor Author

cbalona commented Feb 2, 2021

This is an awesome PR! I wouldn't have been able to come up with something this good.

Thank you John! I am glad you like it.

Seems to only need a resolution for:

  1. the ImportError` - is it a specific version of sklearn that we need to limit to?

I don't think we should limit as it seems that the need to import delayed from sklearn.utils.fixes is temporary, and sklearn intends to remove that function once this issue is resolved: joblib/joblib#1071

As a more permanent fix, I am using delayed from joblib instead--it should do the same thing and all tests pass on my side.

Not needed for the PR, but we should also:
2. Add the class to to docs/modules/classes.rst TOC so its docstring shows up in the API docs
3. Perhaps take your unit test and add a plot to get it as a stand-alone 'Example' in the /examples folder for those that browse the docs by the example gallery
4. Add ensemble.rst to docs/modules/index.rst so the docs show up.
4. Add it to README.rst on the main repo page

Agreed on all the above.

@codecov-io
Copy link

Codecov Report

Merging #116 (a55209a) into master (920502f) will increase coverage by 0.13%.
The diff coverage is 81.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #116      +/-   ##
==========================================
+ Coverage   80.08%   80.22%   +0.13%     
==========================================
  Files          63       68       +5     
  Lines        3465     3570     +105     
  Branches      480      497      +17     
==========================================
+ Hits         2775     2864      +89     
- Misses        508      516       +8     
- Partials      182      190       +8     
Flag Coverage Δ
unittests 80.22% <81.90%> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
chainladder/ensemble/base.py 77.77% <77.77%> (ø)
chainladder/ensemble/voting.py 78.87% <78.87%> (ø)
chainladder/__init__.py 87.50% <100.00%> (+0.83%) ⬆️
chainladder/core/base.py 89.26% <100.00%> (+0.10%) ⬆️
chainladder/ensemble/__init__.py 100.00% <100.00%> (ø)
chainladder/ensemble/tests/test_predict.py 100.00% <100.00%> (ø)
chainladder/ensemble/tests/test_voting.py 100.00% <100.00%> (ø)
chainladder/methods/base.py 91.46% <100.00%> (+2.57%) ⬆️
... and 5 more

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 920502f...a55209a. Read the comment docs.

@jbogaardt jbogaardt merged commit 99cb7e3 into casact:master Feb 2, 2021
@jbogaardt
Copy link
Collaborator

Fantastic contribution @cbalona. Thank you so much!

@RonRichman
Copy link

Thanks everyone - great contribution @cbalona!

jbogaardt pushed a commit that referenced this pull request Feb 3, 2021
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