Repository navigation
feat: Remove Distributions, DataStructures, and MLJModelInterface/Tables dependencies #177
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
Changes from 16 commits
9333a54
4c6e068
c5320b2
0a2092f
1601eae
0d3df58
a3d6ee9
f0a1550
b54e546
eeb5d45
f11c872
7559149
4b8103d
8d469ef
59b41fa
68b1a3f
89f09e7
8291310
2279727
ab44c02
3305b2f
8c04c72
8f502f6
6eadc19
12a1779
2d8d4c0
f8d7ee9
a03bcf0
164bdc6
364143c
c789ffe
a0b79de
638a9a0
130dc81
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| module MCMCDiagnosticToolsDistributionsExt | ||
|
|
||
| using MCMCDiagnosticTools: MCMCDiagnosticTools | ||
| using Distributions: Distributions | ||
|
|
||
| function MCMCDiagnosticTools._rstar_distribution(predictions, ytest, nclasses::Int) | ||
| # create Poisson binomial distribution with support `0:length(predictions)` | ||
| distribution = Distributions.PoissonBinomial(map(Distributions.pdf, predictions, ytest)) | ||
|
|
||
| # scale distribution to support in `[0, nclasses]` | ||
| scaled_distribution = (nclasses//length(predictions)) * distribution | ||
|
|
||
| return scaled_distribution | ||
| end | ||
|
|
||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| module MCMCDiagnosticToolsStatsBaseExt | ||
|
|
||
| using MCMCDiagnosticTools: MCMCDiagnosticTools | ||
| using Statistics: Statistics | ||
| using StatsBase: StatsBase | ||
|
|
||
| function MCMCDiagnosticTools._expectand_proxy(::typeof(StatsBase.mad), x) | ||
| x_folded = MCMCDiagnosticTools._fold_around_median(x) | ||
| return MCMCDiagnosticTools._expectand_proxy(Statistics.median, x_folded) | ||
| end | ||
|
|
||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -147,7 +147,8 @@ within-chain convergence. The training of the classifier can be inspected by adj | |
| If the classifier is deterministic, i.e., if it predicts a class, the value of the ``R^*`` | ||
| statistic is returned (algorithm 1). If the classifier is probabilistic, i.e., if it outputs | ||
| probabilities of classes, the scaled Poisson-binomial distribution of the ``R^*`` statistic | ||
| is returned (algorithm 2). | ||
| is returned (algorithm 2). Probabilistic classifiers require | ||
| [Distributions.jl](https://github.com/JuliaStats/Distributions.jl) to be loaded. | ||
|
|
||
| !!! note | ||
| The correctness of the statistic depends on the convergence of the `classifier` used | ||
|
|
@@ -164,6 +165,7 @@ julia> samples = fill(4.0, 100, 3, 2); | |
| One can compute the distribution of the ``R^*`` statistic (algorithm 2) with a | ||
| probabilistic classifier. | ||
| For instance, we can use a gradient-boosted trees model with `nrounds = 100` sequentially stacked trees and learning rate `eta = 0.05`: | ||
| Because MLJBase already loads Distributions, we don't need to do so ourselves. | ||
|
|
||
| ```jldoctest rstar | ||
| julia> model = EvoTreeClassifier(; nrounds=100, eta=0.05); | ||
|
|
@@ -254,14 +256,22 @@ function _rstar( | |
| length(predictions) == length(ytest) || | ||
| error("numbers of predictions and targets must be equal") | ||
|
|
||
| # create Poisson binomial distribution with support `0:length(predictions)` | ||
| distribution = Distributions.PoissonBinomial(map(Distributions.pdf, predictions, ytest)) | ||
|
|
||
| # scale distribution to support in `[0, nclasses]` | ||
| nclasses = length(MMI.classes(ytest)) | ||
| scaled_distribution = (nclasses//length(predictions)) * distribution | ||
| return _rstar_distribution(predictions, ytest, nclasses) | ||
| end | ||
|
|
||
| # Implemented in MCMCDiagnosticToolsDistributionsExt | ||
| function _rstar_distribution end | ||
|
|
||
| return scaled_distribution | ||
| function _rstar_error_hint(io, exc, argtypes, kwargs) | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In practice this is unreachable, because MMI just defines the interface functions we call but they are implemented in MLJBase (see https://github.com/JuliaAI/MLJModelInterface.jl/blob/f18e0e5692327eda5352caa1430bd5e938898dbe/src/data_utils.jl#L2-L15), which depends on Distributions. So if the user did not yet load MLJBase, they would have already errored before getting to However, MLJBase could make a breaking release dropping Distributions as a direct dependency without MLJModelInterface requiring a breaking release, so this error hint should still exist for that hypothetical future.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another breaking option would be to define our own PoissonBinomial object and return that, documenting only its fields and that it can be converted to a Distributions object, which could be in the extension. Then no error is ever needed.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yet another option, since MMI can't do anything until MLJBase is loaded anyways, would be to move the rstar implementations to an extension weakly depending on either MMI or MLJBase directly. The latter allows us to forego any requirement that users load Distributions, since then we can specify via the compat MLJBase versions that have Distributions as a direct dependency. While MMI is lightweight, removing it and Tables as a direct dependency reduces package load time on my machine from .2s in a script to .06s, I assume because MMI depends on and loads REPL. |
||
| if exc.f === _rstar_distribution | ||
| print( | ||
| io, | ||
| "\n`rstar` with a probabilistic classifier requires Distributions to be loaded, " * | ||
| "e.g. with `using Distributions`.", | ||
| ) | ||
| end | ||
| return nothing | ||
| end | ||
|
|
||
| # unsupported types of predictions and targets | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.