Fix bug in parametric quantile tuning curves #44
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
While slower, it's much simpler to use the quantile function to implement the quantile tuning curve.
Clean up QuadraticDistributionTestCase.test_quantile_tuning_curve and NoisyQuadraticDistributionTestCase.test_quantile_tuning_curve to make them more alike and to make the logic more self-contained. In QuadraticDistributionTestCase.test_quantile_tuning_curve generate the absolute tolerance for the test assertions in the test itself, the same way that it's already done in NoisyQuadraticDistributionTestCase.test_quantile_tuning_curve. In both tests, factor out the number of trials (2,000) and the quantile being tested (0.5) as the variables n_trials and q to make these values easily identifiable.
Fix the quantile_tuning_curve methods of QuadraticDistribution and
NoisyQuadraticDistribution. Both methods use an incorrect formula
for the quantile tuning curve when minimize is True. The tests do
not catch this bug because they only test q = 0.5 and the incorrect
formula happens to work for this one case. In addition, the
test_quantile_tuning_curve tests actually check non-integral
arguments to the quantile tuning curve using an invariant which
would fail when this bug is present, except the invariant was also
entered into the test incorrectly.
To resolve these issues:
* Add a regression test to QuadraticDistributionTestCase and
NoisyQuadraticDistributionTestCase to catch similar bugs in the
future: test_quantile_tuning_curve_with_different_quantiles.
* Fix the invariant used to test non-integral arguments in
test_quantile_tuning_curve on QuadraticDistributionTestCase and
NoisyQuadraticDistributionTestCase so that the tests will pass
when the quantile_tuning_curve methods are correct.
* Fix the bug in QuadraticDistribution.quantile_tuning_curve and
NoisyQuadraticDistribution.quantile_tuning_curve.
This bug does not affect EmpiricalDistribution.quantile_tuning_curve
or its tests as they all use the correct formula.
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.
Fix a bug in the
quantile_tuning_curvemethods ofopda.parametric.QuadraticDistributionandopda.parametric.NoisyQuadraticDistribution. This bug caused the method to return incorrect output wheneverqwasn't equal to0.5and eitherminimizewasTrueorminimizewasNoneandconvexwasTrue.