Closing issue #33. #51
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ain_prop + selection_prop + validation_prop against 1.0.
JanBenisek
suggested changes
Mar 17, 2021
Contributor
JanBenisek
left a comment
There was a problem hiding this comment.
Like I commented, please keep only one test but add more test cases to it.
| selection_prop, | ||
| error_msg) | ||
|
|
||
| def test_train_selection_validation_split_error_correct_prop_no_rounding_error(self): |
Contributor
There was a problem hiding this comment.
Hi, seems like we already have a unit test for that behavior here:
I'd suggest keep only one of the tests (perhaps the original one), BUT add more possible values to test.
| """ | ||
|
|
||
| if train_prop + selection_prop + validation_prop != 1.0: | ||
| if not math.isclose(train_prop + selection_prop + validation_prop, 1.0): |
Contributor
There was a problem hiding this comment.
I like this solution, good idea!
added 3 commits
April 1, 2021 09:46
# Conflicts: # cobra/preprocessing/preprocessor.py
JanBenisek
approved these changes
Apr 1, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Story Title
train/selection/validation split error
Changes made
This proposes a fix for the floating point rounding error in equality checking of the train_prop + selection_prop + validation_prop against 1.0.
The unit test is a little counter-intuitive since it needs to check if a ValueError is NOT raised, but it does carefully test the condition.
How does the solution address the problem
The fixed is based on math.isclose().
Linked issues
Resolves #33