Skip to content

Conversation

@BaptisteCalot
Copy link
Collaborator

@BaptisteCalot BaptisteCalot commented Jul 31, 2024

Description

Adding **predict_params for the predict methods of the models
Fixes #491

Type of change

Please remove options that are irrelevant.

  • 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?

  • Checks that using predict_params during prediction results in conformity scores being all ones for the training data and predictions being all zeros for the test data
  • Checks that predictions are not all zeros when no parameters are passed to the predict method from the custom class
  • Verifies that setting predict_params does not affect the fit_params behavior, ensuring that the model uses only 3 boosting iterations instead of the default 100
  • Ensures that fit_params do not impact predict_params, confirming that predictions are zeros and conformity scores match the expected values
  • Validates that an error is raised if predict_params are used during predict without being specified during fit
  • Validates that an error is raised if predict_params are specified during fit but not used during predict

Checklist

  • I have read the contributing guidelines
  • I have updated the HISTORY.rst and AUTHORS.rst files
  • Linting passes successfully : make lint
  • Typing passes successfully : make type-check
  • Unit tests pass successfully : make tests
  • Coverage is 100% : make coverage
  • Documentation builds successfully : make doc

@BaptisteCalot BaptisteCalot self-assigned this Jul 31, 2024
@BaptisteCalot BaptisteCalot linked an issue Jul 31, 2024 that may be closed by this pull request
Copy link
Collaborator

@LacombeLouis LacombeLouis left a comment

Choose a reason for hiding this comment

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

Hey @BaptisteCalot, thank you for this PR! Just one interrogation that is repeated multiple times.

Copy link
Collaborator

@thibaultcordier thibaultcordier left a comment

Choose a reason for hiding this comment

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

Thank you for your contributions !
I suggest you replace ValueError with UserWarning.

Comment on lines 2112 to 2118
with pytest.raises(ValueError, match=(
r"Using one 'predict_param' in the fit method "
r"without using one 'predict_param' in the predict method. "
r"Please ensure a similar configuration of 'predict_param' "
r"is used in the predict method as called in the fit."
)):
mapie_fitted.predict(X_test)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Test if a UserWarning is raised if better.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey @thibaultcordier, why do you want to raise a warning. We believed an error was more appropriate, since if we don't use predict_params in both, it's clearly an issue.

@LacombeLouis LacombeLouis merged commit 8ddc2a4 into master Sep 3, 2024
@Valentin-Laurent Valentin-Laurent deleted the 491-add-predict-params-into-classification-files branch November 12, 2024 13:37
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.

Add predict params into classification files

3 participants