Add random state to `MapieQuantileRegressor` constructor
Description
Allow users to pass random_state in MapieQuantileRegressor constructor.
When calling fit(), the following logic is applied:
- If a
random_statewas 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_stateis provided neither in fit nor constructor, norandom_stateis used - If a
random_stateis provided in both fit, and constructor, therandom_statefrom 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?
- [x] 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
- [x] I have read the contributing guidelines
- [x] I have updated the HISTORY.rst and AUTHORS.rst files
- [ ] Linting passes successfully :
make lint - [x] Typing passes successfully :
make type-check - [x] Unit tests pass successfully :
make tests - [x] Coverage is 100% :
make coverage - [ ] Documentation builds successfully :
make doc
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.
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!
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
Hello @tiagoleonmelo. Thank you for your effort in contributing to MAPIE.
Few things regarding this issue:
random_stateis actually only used for data splitting (it is present in the code of EnsembleRegressor though, but never used, I'm working right now on a PR to clean this)- We're working on a v1 of MAPIE, and the data splitting will be left to the user, so passing random state to a quantile regressor will be useless
- I acknowledge that being able to pass a random state to control randomness for process other than data splitting could be useful. This could be done in future developments, once v1 is released.
I'm thus closing this PR, but feel free to reopen it or reach out to us if you want to discuss this topic further.