MTN: improve risk control tests #761
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #761 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 55 56 +1
Lines 6093 6325 +232
Branches 351 360 +9
==========================================
+ Hits 6093 6325 +232 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| "best_predict_param_choice, expected", | ||
| [[precision, 0.5], [recall, 0.7]] | ||
| ) | ||
| def test_correct_param_out_of_two(self, best_predict_param_choice, expected): |
There was a problem hiding this comment.
Regarding this test, maybe we can stick to numerical values everywhere (for param and target)? (the dummy_param and dummy_param_2 were misnamed). My point: here we actually care about the param value. In other words, if we change the dummy param/target values, the test may break.
| ) | ||
|
|
||
| dummy_param = 0.5 | ||
| y_calibrate = np.array([1, 0]) |
There was a problem hiding this comment.
We may use a dummy_y here
| dummy_predictions = np.array([[True, False]]) | ||
| valid_params_index = [0] | ||
| controller.valid_predict_params = np.array([dummy_param]) | ||
| controller.valid_predict_params = dummy_param |
There was a problem hiding this comment.
Unsure about using dummy_param here, as it is not obvious that it is a singleton (and that's the point of the test)
|
|
||
| dummy_param = np.array([0.5]) | ||
| dummy_target = 0.9 | ||
| dummy_X_test = [[0]] |
There was a problem hiding this comment.
Could be renamed dummy_X for a more generic use?
| result = bcc_deterministic._get_predictions_per_param( | ||
| X=[], | ||
| params=np.array([0.5]) | ||
| params=dummy_param |
There was a problem hiding this comment.
Unsure about using dummy_param here, as it is not obvious that it is a singleton (and that's the point of the test). Maybe we can create a single_param constant.
There was a problem hiding this comment.
Pull Request Overview
This PR improves the organization and test coverage of risk control tests by separating binary classification control tests into their own dedicated test file and consolidating imports in the precision-recall control test file.
- Move binary classification control tests from the precision-recall test file to a dedicated test file
- Remove unused imports from the precision-recall control test file
- Refactor test code to use common dummy variables and fixtures for better maintainability
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
mapie/tests/risk_control/test_precision_recall_control.py |
Removes binary classification control tests and cleans up unused imports, keeping only precision-recall specific tests |
mapie/tests/risk_control/test_binary_classification_control.py |
New file containing all binary classification control tests with improved organization and shared dummy variables |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| y_calibrate = np.array([1, 0]) | ||
| dummy_predictions = np.array([[True, False]]) | ||
| valid_params_index = [0] | ||
| controller.valid_predict_params = dummy_param |
There was a problem hiding this comment.
The assignment is inconsistent with the expected array type. In the original code (line 808 in the diff), this was assigned as np.array([dummy_param]) but now it's assigned as just dummy_param. This will cause issues when the controller expects an array but receives a scalar value.
Copilot uses AI. Check for mistakes.
| valid_params_index = [0, 1] | ||
|
|
||
| controller.valid_predict_params = np.array( | ||
| [dummy_param, dummy_param + 0.2]) |
There was a problem hiding this comment.
The array construction is incorrect. dummy_param is defined as np.array([0.5]) (line 20), so dummy_param + 0.2 results in np.array([0.7]). The expected values in the parametrized test are 0.5 and 0.7 (scalars), but this creates arrays [0.5] and [0.7]. This should be [dummy_param[0], dummy_param[0] + 0.2] or define dummy_param as a scalar.
| [dummy_param, dummy_param + 0.2]) | |
| [dummy_param[0], dummy_param[0] + 0.2]) |
Copilot uses AI. Check for mistakes.
| predictions_per_param = np.array( | ||
| [[False, False]]) # precision undefined | ||
| valid_params_index = [0] | ||
| controller.valid_predict_params = dummy_param |
There was a problem hiding this comment.
Same issue as in line 218. The assignment should be np.array([dummy_param]) to maintain consistency with the expected array type, or dummy_param should be defined as a scalar if that's the intended usage pattern.
Copilot uses AI. Check for mistakes.
| valid_params_index=valid_params_index | ||
| ) | ||
|
|
||
| assert controller.best_predict_param == dummy_param |
There was a problem hiding this comment.
This assertion will fail because dummy_param is an array [0.5] but best_predict_param is expected to be a scalar 0.5. The assertion should compare with dummy_param[0] or dummy_param should be defined as a scalar.
Copilot uses AI. Check for mistakes.
| predictions_per_param=predictions_per_param, | ||
| valid_params_index=valid_params_index | ||
| ) | ||
| assert controller.best_predict_param == dummy_param |
There was a problem hiding this comment.
Same issue as in line 226. The assertion compares an array with what should be a scalar value. Should be dummy_param[0] or redefine dummy_param as a scalar.
| assert controller.best_predict_param == dummy_param | |
| assert controller.best_predict_param == dummy_param[0] |
Copilot uses AI. Check for mistakes.
No description provided.