Skip to content
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

Make MCMCDiagnosticTools an optional dependency #42

Open
sethaxen opened this issue Sep 21, 2021 · 4 comments
Open

Make MCMCDiagnosticTools an optional dependency #42

sethaxen opened this issue Sep 21, 2021 · 4 comments

Comments

@sethaxen
Copy link
Member

PSIS is generally useful for diagnosing importance sampling, but importance weights are not always generated with MCMC. Since MCMCDiagnosticTools is a somewhat heavy dependency, it's a shame it makes this package less lightweight for non-MCMC applications.

We could instead make MCMCDT a conditional dependency using Requires. I propose something like the following methods:

psis(log_ratios; kwargs...): computes PSIS assuming r_eff=1
psis(log_ratios, r_eff; kwargs...): computes PSIS using the user-provided r_eff.
psis(log_ratios; ess_method::MCMCDiagnosticTools.AbstractESSMethod, kwargs...): defined if MCMCDiagnosticTools.jl is loaded using Requires; computes r_eff using the provided ESS method and calls psis(log_ratios, r_eff; kwargs...)

@devmotion
Copy link
Member

Since MCMCDiagnosticTools is a somewhat heavy dependency, it's a shame it makes this package less lightweight for non-MCMC applications.

Even without the MCMCDiagnosticTools dependency this package is far from being lightweight:

AxisKeys = "94b1ba4f-4ee9-5380-92f1-94cde586c3c5"
Distributions = "31c24e10-a181-5473-b8eb-7969acd0382f"
DocStringExtensions = "ffbed154-4ef7-542d-bbb7-c09d3a79fcae"
FFTW = "7a1cc6ca-52ef-59f5-83cd-3a7055c09341"
InteractiveUtils = "b77e0a4c-d291-57a0-90e8-8db25a27a240"
LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e"
LoopVectorization = "bdcacae8-1622-11e9-2a5c-532679323890"
MCMCDiagnosticTools = "be115224-59cd-429b-ad48-344e309966f0"
NamedDims = "356022a1-0364-5f58-8944-0da4b18d706f"
Polyester = "f517fe37-dbe3-4b94-8317-1923a5111588"
PrettyTables = "08abe8d2-0d0c-5749-adfa-8a2ac140af0d"
Printf = "de0858da-6303-5e67-8744-51eddeeeb8d7"
Random = "9a3f8284-a2c9-5f02-9a11-845980a1fd5c"
Requires = "ae029012-a4dd-5104-9daa-d747884805df"
Statistics = "10745b16-79ce-11e8-11f9-7d13ad32a3b2"
StatsBase = "2913bbd2-ae8a-5f71-8c99-4fb6c76f3a91"
StatsFuns = "4c63d2b9-4356-54db-8cca-17b64c39e42c"
Tullio = "bc48ee85-29a4-5162-ae0b-a64e1601d4bc"

Maybe this could/should be changed but since I haven't used ParetoSmooth and don't plan to in the near future, others should discuss and decide this.

@sethaxen
Copy link
Member Author

Yes, this is one of several dependency-related issues, see #44

@devmotion
Copy link
Member

Since MCMCDiagnosticTools is a somewhat heavy dependency, it's a shame it makes this package less lightweight for non-MCMC applications.

Actually, I think it is not a heavy dependency: #44 (comment)

@sethaxen
Copy link
Member Author

Hm, I seem to recall it taking longer to load in the past. I wonder if invalidations were recently fixed in a dependency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants