search

dssg / triage

General Purpose Risk Modeling and Prediction Toolkit for Policy and Social Good Problems
Other
182 stars 61 forks source link

Add BaselineRankMultiFeature #871

Closed shaycrk closed 2 years ago

shaycrk commented 2 years ago

Building on #870, this PR adds BaselineRankMultiFeature to allow ranking by more than one feature as a baseline model (if we decide on a different name for the sort order parameter there, we'll want to be sure to rename it here as well for consistency)

ecsalomon commented 2 years ago

No further thoughts beyond what @thcrock has said but wondering if we should just deprecate PercentileRankOneFeature with this since it obviates the need for that ranker.

shaycrk commented 2 years ago

@ecsalomon -- I'm fine either keeping or deprecating PercentileRankOneFeature. I agree this should fit most of the use cases for that, but unlike PercentileRankOneFeature, it doesn't attempt to ensure that the scores are interpretable as percentiles, just correctly ordered, so we would be loosing some functionality. I'm not sure how often that feature gets used, though.

ecsalomon commented 2 years ago

Yeah, I mean, there was no real motivation for percentile being the method there. A deprecation warning might surface anyone who is actually using them as percentiles downstream somewhere.

Digging in a bit more, I might actually argue for the generic scaler to be the thing that controls how scores from the rankers get scaled (i.e., they return whatever scale is applied to the input features) rather than the rankers applying their own scaling.

shaycrk commented 2 years ago

That makes sense about the percentiles, I can certainly add a deprecation warning here and direct people to the newer ranking method.

On the scaler, how would the generic scaler work when you're ranking across multiple features? Even with one feature, if you're learning the scaling from the training set, you'll also run into cases where the test values are outside of [0,1] and we might not want to cap them for the purposes of simple ranking -- of course, the 0 to 1 range is a bit arbitrary here, but if we are scaling to something like that it's probably good to keep them within the range.

ecsalomon commented 2 years ago

Yeah, a fair point about not artificially capping test ranks if they fall outside the range. I'm convinced I was wrong. :)

shaycrk commented 2 years ago

@thcrock -- just pinging if you have other thoughts on the unit tests here?

Also, @rayidghani -- other thoughts on the descend parameter name? I don't think low_value_high_score is great, but also don't have better ideas.

shaycrk commented 2 years ago

Going ahead and merging this now, but we can revisit the parameter name and unit tests in future pull request(s) if we want.

Closes #869