-
Notifications
You must be signed in to change notification settings - Fork 6
add continuous boyce index #52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #52 +/- ##
==========================================
- Coverage 92.22% 91.54% -0.68%
==========================================
Files 14 14
Lines 720 769 +49
==========================================
+ Hits 664 704 +40
- Misses 56 65 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/probabilistic.jl
Outdated
| "$ContinuousBoyceIndexDoc" | ||
| ContinuousBoyceIndex | ||
| "$ContinuousBoyceIndexDoc" | ||
| cbi(x, y; kw...) = ContinuousBoyceIndex(; kw...)(x, y) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ablaom what would be the right way to go here? I know the other functions don't have this interface, but here I think it would make a lot of sense to allow cbi(ŷ, y; n_bins=5)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, you have to make n_bins part of the struct. So you do ContinuousBoyceIndex(nbins=5)(yhat, y).
However, if you want, you can define a pure functional version Functions.continuous_boyce_index here and refactor so that your struct version calls that. And then documentation can point out the core implementation, like we do for MatthewsCorrelation.
|
Sorry, forgot about this one a little bit but let's finish it :). I cleaned up some code and moved the main thing to functions.jl as you suggested. The funny thing about the CBI is that there are so many implementations and most of them are either a little weird or slightly wrong. But I've looked at the original at the original paper to make sure this implementation follows their idea very closely. |
|
This is looking pretty good. Thanks for the follow through. There's some missing coverage for some corner-case (?) logic in the core function. Be great if you can address that. |
src/probabilistic.jl
Outdated
|
|
||
| ContinuousBoyceIndex(; kw...) = _ContinuousBoyceIndex(; kw...) |> robust_measure |> fussy_measure | ||
|
|
||
| function (m::_ContinuousBoyceIndex)(ŷ::UnivariateFiniteArray, y::NonMissingCatArrOrSub; warn=true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type annotation for yhat is too strict. Either remove it altogether, or you could do ::AbstractArray{<:UnivariateFinite}. For consider
julia> y = categorical(rand("ab", 10), ordered=true);
julia> d = CategoricalDistributions.Distributions.fit(UnivariateFinite, y);
julia> yhat = fill(d, 10);
julia> ContinuousBoyceIndex()(yhat, y)
ERROR: MethodError: no method matching (::StatisticalMeasures._ContinuousBoyceIndex)(::Vector{…}, ::CategoricalVector{…})
The object of type `StatisticalMeasures._ContinuousBoyceIndex` exists, but no method is defined for this combination of argument types when trying to treat it as a callable object.|
Are you absolutely sure you want this julia> cbi(yhat, y)
[ Info: removing 30 bins without any observations
0.5410565658852077 |
src/probabilistic.jl
Outdated
| kind_of_proxy=StatisticalMeasures.LearnAPI.Distribution(), | ||
| orientation=Score(), | ||
| external_aggregation_mode=Mean(), | ||
| human_name = "continuous boyce index", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is Boyce not a proper name?
| human_name = "continuous boyce index", | |
| human_name = "continuous Boyce index", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to have this. Just a few minor points.
I'd be inclined to call the core function Functions.continuous_boyce_index as we're generally verbose about those, but I'll leave it up to you.
Note to self: I have reviewed the traits.
Co-authored-by: Anthony Blaom, PhD <[email protected]>
yeah maybe not. Let me think about that. Thanks for the thorough review |
|
Okay I ended up changes a few more things:
This measure is really nice for presence-only models, but it's pretty weird you can get such different values from it based on some of these parameters (and that there aren't really any agreed-upon defaults). I've kept the name at Functions.cbi - we also have Functions.auc and the full name is pretty long |
closes #51
Todo: