Fix Inconsistent Test Outcome from TargetDistributionDataCheck - #2608
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2608 +/- ##
=======================================
+ Coverage 99.9% 99.9% +0.1%
=======================================
Files 295 295
Lines 26847 26848 +1
=======================================
+ Hits 26801 26802 +1
Misses 46 46
Continue to review full report at Codecov.
|
| if log_detected: | ||
| details = { | ||
| "shapiro-statistic/pvalue": f"{round(shapiro_test_og.statistic, 3)}/{round(shapiro_test_og.pvalue, 3)}" | ||
| "shapiro-statistic/pvalue": f"{round(shapiro_test_og.statistic, 2)}/{round(shapiro_test_og.pvalue, 3)}" |
There was a problem hiding this comment.
Based on the previous implementation, 4-5 out of 100 runs would result in slightly different values due to how rounding would occur after a certain number of decimal places. This would throw off the statistic to the thousandths place by 1, resulting in a failure. I'm rounding all values here to keep the results the same in the data check and the test block.
There was a problem hiding this comment.
Sweet. If the value of this ratio impacted whether or not we did the log transform, then I would have suggested we instead do the rounding in the test as opposed to the implementation. Since this is just to provide extra details to users, I think it's ok to round however we want!
| y = [-1, -3, -5, 4, -2, 4, -4, 2, 1, 1] | ||
| else: | ||
| y = np.random.normal(4, 1, 100) | ||
| y = [9.2, 7.66, 4.93, 3.29, 4.06, -1.28, 4.95, 6.77, 9.07, 7.67] |
There was a problem hiding this comment.
Both these test blocks would previously throw errors 7-8 times out of a 100 runs. Even after changing the values to be more gaussian in distribution with larger sample sizes, they still failed 1-2 times out of the 100,000 simulations I ran. Randomness leads to a somewhat lognormal distribution every once in a while. I decided to replace these with constant smaller samples of a lognormal distribution so we don't have to worry about inconsistency again.
freddyaboulton
left a comment
freddyaboulton
left a comment
There was a problem hiding this comment.
Thank you @ParthivNaresh !!
| >>> X = None | ||
| >>> y = lognorm.rvs(0.1, size=1000) | ||
| >>> target_checdk = TargetDistributionDataCheck() | ||
| >>> y = [0.946, 0.972, 1.154, 0.954, 0.969, 1.222, 1.038, 0.999, 0.973, 0.897] |
There was a problem hiding this comment.
👍 agree that it's best to not rely on random vaues for this
| if log_detected: | ||
| details = { | ||
| "shapiro-statistic/pvalue": f"{round(shapiro_test_og.statistic, 3)}/{round(shapiro_test_og.pvalue, 3)}" | ||
| "shapiro-statistic/pvalue": f"{round(shapiro_test_og.statistic, 2)}/{round(shapiro_test_og.pvalue, 3)}" |
There was a problem hiding this comment.
Sweet. If the value of this ratio impacted whether or not we did the log transform, then I would have suggested we instead do the rounding in the test as opposed to the implementation. Since this is just to provide extra details to users, I think it's ok to round however we want!
| y = [-1, -3, -5, 4, -2, 4, -4, 2, 1, 1] | ||
| else: | ||
| y = np.random.normal(4, 1, 100) | ||
| y = [9.2, 7.66, 4.93, 3.29, 4.06, -1.28, 4.95, 6.77, 9.07, 7.67] |
3 participants
Fixes #2581
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.rstto include this pull request by adding :pr:123.