MAPIE icon indicating copy to clipboard operation
MAPIE copied to clipboard

Add random state to `MapieQuantileRegressor` constructor

Open tiagoleonmelo opened this issue 1 year ago • 3 comments

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?

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

tiagoleonmelo avatar Mar 22 '24 13:03 tiagoleonmelo

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 avatar Mar 25 '24 12:03 LacombeLouis

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!

LacombeLouis avatar May 13 '24 14:05 LacombeLouis

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

tiagoleonmelo avatar Jun 02 '24 08:06 tiagoleonmelo

Hello @tiagoleonmelo. Thank you for your effort in contributing to MAPIE.

Few things regarding this issue:

  • random_state is 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.

Valentin-Laurent avatar Jan 13 '25 10:01 Valentin-Laurent