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

First instance of mapie classifier #79

Merged
merged 6 commits into from Aug 2, 2021
Merged

First instance of mapie classifier #79

merged 6 commits into from Aug 2, 2021

Conversation

aagoumbala
Copy link
Contributor

@aagoumbala aagoumbala commented Jul 28, 2021

Description

First instance of mapie classifier with the fit and predict methods, the corresponding checks and the test file.

Type of change

Please check options that are relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

The tests are carried out on the classification.py module with test_classification.py (see file for more details)

Checklist:

  • I have read the contributing guidelines
  • I have updated the HISTORY.rst and AUTHORS.rst files
  • Linting passes successfully : flake8 . --exclude=doc
  • Typing passes successfully : mypy mapie examples --strict
  • Unit tests pass successfully : pytest -vs --doctest-modules mapie
  • Coverage is 100% : pytest -vs --doctest-modules --cov-branch --cov=mapie --pyargs mapie
  • Documentation builds successfully : cd doc; make clean; make html

@aagoumbala aagoumbala changed the title mapie classifier first instance First instance mapie classifier Jul 28, 2021
@aagoumbala aagoumbala requested a review from vtaquet July 28, 2021 15:52
@codecov-commenter
Copy link

codecov-commenter commented Jul 28, 2021

Codecov Report

Merging #79 (3a2557f) into dev (675f151) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##               dev       #79    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files            9        11     +2     
  Lines          554       811   +257     
  Branches        42        54    +12     
==========================================
+ Hits           554       811   +257     
Impacted Files Coverage Δ
mapie/__init__.py 100.00% <100.00%> (ø)
mapie/classification.py 100.00% <100.00%> (ø)
mapie/regression.py 100.00% <100.00%> (ø)
mapie/tests/test_classification.py 100.00% <100.00%> (ø)
mapie/tests/test_utils.py 100.00% <100.00%> (ø)
mapie/utils.py 100.00% <100.00%> (ø)

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 675f151...3a2557f. Read the comment docs.

@aagoumbala aagoumbala changed the title First instance mapie classifier First instance of mapie classifier Jul 28, 2021
mapie/classification.py Outdated Show resolved Hide resolved
mapie/classification.py Outdated Show resolved Hide resolved
mapie/classification.py Outdated Show resolved Hide resolved
mapie/classification.py Outdated Show resolved Hide resolved
mapie/classification.py Outdated Show resolved Hide resolved
mapie/classification.py Outdated Show resolved Hide resolved
mapie/classification.py Outdated Show resolved Hide resolved
mapie/classification.py Outdated Show resolved Hide resolved
mapie/classification.py Outdated Show resolved Hide resolved
mapie/classification.py Outdated Show resolved Hide resolved
mapie/classification.py Outdated Show resolved Hide resolved
@vtaquet
Copy link
Member

vtaquet commented Jul 29, 2021

Beau boulot pour l'initiation de MapieClassifier ! Quelques commentaires généraux:

  • je propose de déplacer les méthodes identiques à celles de MapieRegressor dans utils.py afin d'éviter les doublons
  • cv ne doit finalement prendre que deux arguments pour le moment car la validation croisée n'est pas prévue pour tout de suite
  • vérifie bien que le taux de couverture est de 100%, ce qui t'assure que toutes les lignes du module ont au moins été vérifiées une fois
  • peux-tu commencer à modifier la documentation afin qu'elle prenne en compte la partie classification ? Comme discuté, il faut ajouter des fichiers de type tutorial_classification.rst et theoretical_description_classification.rst à indexer dans le fichier index.rst.

@aagoumbala
Copy link
Contributor Author

J'ai pris en compte les différents commentaires. Il reste à regrouper les fonctions identiques de mapieclassifier et mapieregression dans utils.

@vtaquet vtaquet added the enhancement New feature or request label Jul 30, 2021
@aagoumbala
Copy link
Contributor Author

J'ai regroupé les fonctions identiques dans utils.

mapie/classification.py Outdated Show resolved Hide resolved
mapie/utils.py Show resolved Hide resolved
mapie/classification.py Outdated Show resolved Hide resolved
mapie/classification.py Outdated Show resolved Hide resolved
cv: Optional[str]
The cross-validation strategy for computing scores :

- ``None``, to use mapie for fitting.
Copy link
Member

Choose a reason for hiding this comment

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

Décris un peu plus explicitement ce que ça va faire : "None, MapieClassifier will be used for fitting the base model" ou quelque chose comme ça.

@vtaquet vtaquet merged commit 6dd66df into dev Aug 2, 2021
@vtaquet vtaquet deleted the mapieclassifier-dev branch August 2, 2021 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants