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

Fix range of styling issues identified by precommit config #100

Merged
merged 5 commits into from
Mar 15, 2021

Conversation

timvink
Copy link
Contributor

@timvink timvink commented Mar 12, 2021

#98

@timvink timvink requested a review from Matgrb March 12, 2021 20:48
Copy link
Contributor

@Matgrb Matgrb left a comment

Choose a reason for hiding this comment

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

Looks good. Only make sure that the unit tests succeed

@timvink
Copy link
Contributor Author

timvink commented Mar 15, 2021

@Matgrb I cannot find this bug :(. Somehow, a single value changed from 0 to 0.5. And in only 1 of the tests. As far as I can see, I only changed docstrings and other styling issues. Can you have a look? You wrote the code, perhaps you know better where to look / where it could potentially change the shap value.

@Matgrb Matgrb merged commit 81fd25e into main Mar 15, 2021
@timvink
Copy link
Contributor Author

timvink commented Mar 15, 2021

Thnx Mateusz! Still don't know what the issue was but trust it's fine :)

@Matgrb
Copy link
Contributor

Matgrb commented Mar 16, 2021

It was a weird shap related issue.

initially this test was checking if the mean shap value was 0.5. Then with some updates, the value has suddenly changed to 0. Now it is back at 0.5.

I have decided to disregard the mean shap value for now, since it is the mean abs shap value that is the most important there.
In the future, we can replace the shap based unit tests with mocks to ensure full stability.

@timvink timvink deleted the 98-precommit-issue branch March 17, 2021 09:01
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