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
Encode targets for Time Series Classification Pipelines #2040
Conversation
f8bebda
to
d7d964b
Compare
Codecov Report
@@ Coverage Diff @@
## main #2040 +/- ##
=========================================
- Coverage 100.0% 100.0% -0.0%
=========================================
Files 278 278
Lines 22920 22770 -150
=========================================
- Hits 22911 22761 -150
Misses 9 9
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Can you add a test for thresholding the binary TS pipeline? As per issue, "then adding in an equivalent test as test_binary_pipeline_string_target_thresholding
in test_pipelines
to the time_series_pipeline
tests."
Otherwise, looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Nice mocking work
d7d964b
to
ad29895
Compare
844a765
to
74704a1
Compare
@bchen1116 Great call! Just pushed up that test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, love the tests too. I dunno what it is about a good test that I can look at the name of and read and see it do what it set out to do that makes me feel good, but it does. I added a note about maybe parameterizing your for loop to make test failures a little prettier, but take it or leave it.
for pipeline_class in [time_series_binary_classification_pipeline_class, | ||
logistic_regression_binary_pipeline_class]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be a nit, but it'll look better in test failure, perhaps paratetrize this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call! I just got lazy cause you can't parametrize over fixtures lol
Pull Request Description
Fixes #1976
After creating the pull request: in order to pass the release_notes_updated check you will need to update the "Future Release" section of
docs/source/release_notes.rst
to include this pull request by adding :pr:123
.