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 random state to MapieQuantileRegressor constructor #424

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

tiagoleonmelo
Copy link

@tiagoleonmelo tiagoleonmelo commented Mar 22, 2024

Description

Allow users to pass random_state in MapieQuantileRegressor constructor.

When calling fit(), the following logic is applied:

  • If a random_state was set for this instance, and no random_state is passed in the function call, use the random state set in the constructor
  • If no random_state is provided neither in fit nor constructor, no random_state is used
  • If a random_state is provided in both fit, and constructor, the random_state from fit is used

My reasoning is that the random_state provided in fit will overwrite the one previously provided. This doesn't feel super intuitive to me (maybe the random state should not be passable now during fit to not have random state mismatches?), but I think it is better compared to the alternative

Fixes #405

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • Added a small unit test to make sure that the random state that is set when instancing results in the same output as a random state set during fit(), mimicking the desired behavior

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

@LacombeLouis LacombeLouis self-requested a review March 25, 2024 08:42
@LacombeLouis
Copy link
Collaborator

Hey @tiagoleonmelo,
Thank you for this PR, note that it seems like the unit tests are not all passed. Make sure to check your linting ;)
We will take a look at the PR as soon as all the tests have passed.

@LacombeLouis
Copy link
Collaborator

Hey @tiagoleonmelo,
Thank you for this PR! For the moment it doesn't pass some tests, but we have fixed the issue. So you can git merge upstream/main and everything should work!
Thank you!

@tiagoleonmelo
Copy link
Author

tiagoleonmelo commented Jun 2, 2024

Hey @LacombeLouis, sorry about the delay. I synced my fork, but still running into a few issues which I don't think come from my changes - getting a 404 somewhere

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.

Missing random state in Quantile Regression
2 participants