Skip to content

Small fix to make time series featurizer deterministic#3384

Merged
bchen1116 merged 8 commits intomainfrom
bc_ts_deterministic
Mar 21, 2022
Merged

Small fix to make time series featurizer deterministic#3384
bchen1116 merged 8 commits intomainfrom
bc_ts_deterministic

Conversation

@bchen1116
Copy link
Contributor

@bchen1116 bchen1116 commented Mar 16, 2022

After this fix, we see this in the perf tests:
image
Ultimately, we note that the before and after scores for both of these pipelines are the same, so there could be an issue with how we're storing the full rankings or finding the order in LG. However, this PR is focused on making TS deterministic.

The full, fixed report is below:
report_ts.html.zip

The perf tests look good! Seems like the behavior is now the same between runs

@bchen1116 bchen1116 self-assigned this Mar 16, 2022
@codecov
Copy link

codecov bot commented Mar 16, 2022

Codecov Report

Merging #3384 (df826f2) into main (9d1d5c9) will increase coverage by 0.1%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##            main   #3384     +/-   ##
=======================================
+ Coverage   99.6%   99.6%   +0.1%     
=======================================
  Files        329     329             
  Lines      32231   32241     +10     
=======================================
+ Hits       32102   32112     +10     
  Misses       129     129             
Impacted Files Coverage Δ
...ansformers/preprocessing/time_series_featurizer.py 100.0% <100.0%> (ø)
.../tests/pipeline_tests/test_time_series_pipeline.py 99.9% <100.0%> (+0.1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d1d5c9...df826f2. Read the comment docs.

Copy link
Collaborator

@jeremyliweishih jeremyliweishih left a comment

Choose a reason for hiding this comment

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

Left a comment but looks great!

Copy link
Contributor

@freddyaboulton freddyaboulton left a comment

Choose a reason for hiding this comment

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

Thank you @bchen1116 !!

@bchen1116 bchen1116 merged commit 01da909 into main Mar 21, 2022
@chukarsten chukarsten mentioned this pull request Mar 25, 2022
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.

3 participants