more checks for inconsistent feature names #47
Merged
Conversation
This file contains 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
Currently throws out anything that doesn't exactly match previous feature names. A better solution might be to reorder features if features_names are out of order. Also could make np.nan in features if feature_names are missing
Accidentally removed the check. Instead of putting back into add_samplet, moved the code into _check_features. DRYs up the check in __setitem__
WillForan
changed the title
|
Thanks a lot Will for this PR - let me take a look |
raamana
approved these changes
Apr 2, 2021
|
Sorry for the delay in reviewing @WillForan! Thanks very much for this useful PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
I may have totally misunderstood the structure of
add_samplet.It looks like
_datawas modified beforefeature_nameswas checked which lead to #45 . I pulled the checks out and into_check_feature_namesand also pushed a ValueError for feature dim mismatch into_check_features(to fix the tests I broke).I think the same mismatch issue might exist for
attrs. But I didn't want to work on that yet -- in case I totally missed the point.I added two tests that check for a new exception type
InvalidFeatureNamesException.But reviewing it now, those should maybe beValueErrorto be consistent with the other add_samplet errors (?)As part of the tests, I created a decorator
forall_dataset_typesto avoid putting a for loop around each test. I'm not sure if that will make debugging the tests harder. (They made writing them marginally easier)Happy to revise if I'm not way off base. But feel free to cherry pick if any of it could be useful too.