add more robust solution for _check_and_update_repeating_values #166
Conversation
There was a problem hiding this comment.
Pull request overview
This PR attempts to re-add a check for unchanging values by modifying the validation condition from > to >= on line 60. However, this change introduces a critical bug that breaks existing functionality for handling repeating values in numeric distributions.
Key Issue:
- The validation change rejects equal consecutive values before they can be processed by the
_check_and_update_repeating_valuesmethod
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if percentiles[i].percentile >= percentiles[i + 1].percentile: | ||
| raise ValueError("Percentiles must be in strictly increasing order") | ||
| if percentiles[i].value > percentiles[i + 1].value: | ||
| if percentiles[i].value >= percentiles[i + 1].value: |
There was a problem hiding this comment.
This change breaks the handling of repeating values. The validation now rejects equal values with >=, but the _check_and_update_repeating_values method (called on line 81) is specifically designed to handle and modify repeating values at bounds. With this change, repeating values will be rejected before they can be processed, causing the tests test_valid_repeating_values_at_lower_bound and test_valid_repeating_values_at_upper_bound to fail.
If the intention is to truly reject all equal values, then the _check_and_update_repeating_values method should be removed. Otherwise, this validation should remain as > to allow repeating values to be processed by the update method.
| if percentiles[i].value >= percentiles[i + 1].value: | |
| if percentiles[i].value > percentiles[i + 1].value: |
lsabor
changed the title
No description provided.