search

dssg / triage

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

Renaming PercentileRankOneFeature descend parameter (closes #869) #870

Closed shaycrk closed 2 years ago

shaycrk commented 2 years ago

Renaming the descend parameter from PercentileRankOneFeature to high_score_low_value to try to be more explicit about what it's doing (but certainly also open to other names if anyone has better ideas).

Another question for review: should we keep (but raise a deprecation warning) the descend parameter as well to ensure backwards compatibility with existing triage configs?

shaycrk commented 2 years ago

@ecsalomon -- makes sense, I went ahead and renamed it that way.

Thoughts on the idea of keeping the descend parameter as well but raise a deprecation warning?

ecsalomon commented 2 years ago

Seems like a reasonable choice if there are projects that might upgrade

shaycrk commented 2 years ago

Thanks, @ecsalomon! Went ahead and added descend back with a deprecation warning.

@rayidghani / @thcrock -- let me know what you think here as well (especially if there's a better parameter name; I think this one is clear enough, but feels a little "clunky")

thcrock commented 2 years ago

I don't have a better parameter name here. I had to go into the code comments to figure out what the changed parameter meant. Is this class described in some other documentation that should be updated? If not, should it be added to some other documentation?

ecsalomon commented 2 years ago

I think it's mentioned in one of the example configs... maybe elsewhere

shaycrk commented 2 years ago

Yeah, it's in the example experiment config, but I don't think any of the catwalk estimators are described in the actual documentation, unfortunately. Any objections to my creating an issue for that for now and getting back to it later?

shaycrk commented 2 years ago

Closing without merging -- superseded by #871