FEAT export InterpolatedThresholder as a public object and update its API (e.g., rename an argument) #918
Conversation
… example Signed-off-by: Roman Lutz <rolutz@microsoft.com>
Signed-off-by: Roman Lutz <rolutz@microsoft.com>
Signed-off-by: Roman Lutz <rolutz@microsoft.com>
Signed-off-by: Roman Lutz <rolutz@microsoft.com>
Signed-off-by: Roman Lutz <rolutz@microsoft.com>
Signed-off-by: Roman Lutz <rolutz@microsoft.com>
Signed-off-by: Roman Lutz <rolutz@microsoft.com>
Signed-off-by: Roman Lutz <rolutz@microsoft.com>
Signed-off-by: Roman Lutz <rolutz@microsoft.com>
romanlutz
added
enhancement
MiroDudik
changed the title
| @@ -21,14 +21,13 @@ class InterpolatedThresholder(BaseEstimator, MetaEstimatorMixin): | |||
|
|
|||
| At prediction time, the predictor takes as input both standard and sensitive features. | |||
| Based on the values of sensitive features, it then applies a randomized thresholding | |||
| transformation according to the provided `interpolation_dict`. | |||
| transformation according to the provided `threshold_interpolation`. | |||
There was a problem hiding this comment.
why this change? if you don't like the original, we should go for something more concise. maybe something like threshold_info or thresholding_info or even thresholds (but that is a bit confusing, because it sounds like the structure would be simpler).
There was a problem hiding this comment.
Info isn't a great name. Everything is "info", yet we don't call "width" variables "width_info". It's a stylistic improvement as well as a description improvement: interpolation and threshold are the two important things worth mentioning IMO.
If you don't think it's an improvement I don't mind scrapping it, though (not the hill I choose to die on). It was originally part of an older PR about documenting post processing (which I'm actively working on) and got refactored out into its own change for simplicity.
There was a problem hiding this comment.
How about threshold_weights since basically each value of a threshold gets a certain probabilistic weight?
There was a problem hiding this comment.
I don't dislike it, but that's just p0 and p1 🙂
It's really not a bad name though...
|
|
||
| Parameters | ||
| ---------- | ||
| estimator : | ||
| base estimator | ||
|
|
||
| interpolation_dict : dict | ||
| threshold_interpolation : dict |
There was a problem hiding this comment.
See my comment above re. naming. By the way, do we like the structure of this argument? The main reason why I've originally excluded this estimator from the module was that I wasn't sure we all liked this deep structure and I didn't want to commit to it... I usually prefer "shallower" input arguments.
| from ._threshold_optimizer import ThresholdOptimizer # noqa: F401 | ||
| from ._plotting import plot_threshold_optimizer # noqa: F401 | ||
|
|
||
| __all__ = [ | ||
| "ThresholdOptimizer", | ||
| "plot_threshold_optimizer" | ||
| "InterpolatedThresholder", |
There was a problem hiding this comment.
You'll also want to export ThresholdOperation. That's another structured object, where we should review whether we like its current API enough to make it external.
Adding to the list: tests! |
This PR does the following:
interpolation_dictto the more descriptive namethreshold_interpolation.InterpolatedThresholderinfairlearn.postprocessing.