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 explicit state setting #242

Merged
merged 19 commits into from
Mar 28, 2024
Merged

Add explicit state setting #242

merged 19 commits into from
Mar 28, 2024

Conversation

ReinierKoops
Copy link

@ReinierKoops ReinierKoops commented Mar 18, 2024

Set the random states explicitly.

Tasks:

@ReinierKoops
Copy link
Author

This PR depends on changes from #239 (Py v312) - so should be merged after.

@ReinierKoops
Copy link
Author

The code changes are a general test code cleanup. It should try to reuse parts of the code that before were duplicates. Also the seeds are now set as a fixture and used more "globally" that way.

@ReinierKoops
Copy link
Author

Also, verbose logging is simplified, however, in the code it does not seem to be used much.

Maybe at some point later we can discard verbose logging altogether and replace it by loguru's INFO DEBUGGING etc way of logging. However, that idea should maybe be discussed at a later time.

@ReinierKoops
Copy link
Author

We should make a more thorough check keeping this in mind: #221 - if we apply whats discussed here I'd imagine much of our test code will be a bit easier and more clean. Will have another look at the test (prolly tomorrow).

@ReinierKoops ReinierKoops changed the title add explicit state setting WIP: add explicit state setting Mar 18, 2024
@ReinierKoops
Copy link
Author

This PR should solve this issue: #240

@ReinierKoops ReinierKoops changed the title WIP: add explicit state setting Add explicit state setting Mar 19, 2024
@ReinierKoops ReinierKoops changed the title Add explicit state setting WIP: Add explicit state setting Mar 19, 2024
@ReinierKoops ReinierKoops changed the title WIP: Add explicit state setting Add explicit state setting Mar 19, 2024
@ReinierKoops
Copy link
Author

@adri0 its ready for review and includes fixes for multiple issues.

@ReinierKoops ReinierKoops changed the title Add explicit state setting [WIP] Add explicit state setting Mar 26, 2024
@ReinierKoops ReinierKoops changed the title [WIP] Add explicit state setting Add explicit state setting Mar 26, 2024
@@ -1210,7 +1209,8 @@ def _get_fit_params_lightGBM(
"eval_set": [(X_val, y_val)],
"callbacks": [early_stopping(self.early_stopping_rounds, first_metric_only=True)],
}
if self.verbose >= 100:

if self.verbose >= 2:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not >1 like the comparisons above? ;)

Copy link
Author

Choose a reason for hiding this comment

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

because it was a three tiered system. I think we can maybe reduce it even further. Feel free to make a issue!

Comment on lines +169 to +172
"""
Fixture.
"""
return pd.Series([1, 0, 1, 0], index=[1, 2, 3, 4])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe remove those """Fixture """ docstrings?

Copy link
Author

Choose a reason for hiding this comment

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

removed in latest pr

@@ -237,22 +173,18 @@ def test_shap_interpret_fit_compute(fitted_tree, X_train, y_train, X_test, y_tes


@pytest.mark.skipif(os.environ.get("SKIP_LIGHTGBM") == "true", reason="LightGBM tests disabled")
def test_shap_interpret_complex_data(complex_data_split, complex_fitted_lightgbm):
def test_shap_interpret_complex_data(complex_data_split_with_categorical, complex_fitted_lightgbm, random_state):
"""
Test lightgbm.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also seem like redundant docstring.

Copy link
Author

@ReinierKoops ReinierKoops Mar 28, 2024

Choose a reason for hiding this comment

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

removed all the redundant/not explanatory docstrings from all tests in latest pr

@ReinierKoops
Copy link
Author

The comments will be fixed in the latest PR. This will be merged.

@ReinierKoops ReinierKoops removed the request for review from PaulZhutovsky March 28, 2024 05:26
@ReinierKoops ReinierKoops merged commit c1285c3 into main Mar 28, 2024
15 checks passed
ReinierKoops pushed a commit that referenced this pull request Apr 11, 2024
…, removed unused code and updated notebooks to work (again). (#248)

This PR depends on the PR to be accepted:
#242

______

The PR fixes the following:

- Removed unused code and so partially fixed:
#245
- Updates all notebooks so they are all working:
#246
- Add explicit support for regressors and multiclass: :
#241 & :
#169
- Updated the yaml files for github actions. Now we have a weekly
cronjob (instead of daily) to test the notebooks.
- Enable tests which were previously failing or only enabled in certain
circumstances (much has changed the last 3 years with regards of
adoption of the Mac ARM architecture support)
- Removed most (if not all except one on purpose) of the copyright
notice in code.

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
ReinierKoops pushed a commit that referenced this pull request Apr 23, 2024
…252)

This PR depends on the PR to be accepted:
#248

______

This cleanup removes some more unused code and simplifies parts of our
implementations. It should allow for a boost in performance for bigger
use-cases, although minimal.

Also fix:

- [x] #242 comments 
- [x] #255
- [x] #245
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.

2 participants