-
Notifications
You must be signed in to change notification settings - Fork 220
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
Clean up exports #2474
base: master
Are you sure you want to change the base?
Clean up exports #2474
Conversation
3eec503
to
6a8d318
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2474 +/- ##
===========================================
- Coverage 85.01% 56.43% -28.59%
===========================================
Files 21 21
Lines 1582 1561 -21
===========================================
- Hits 1345 881 -464
- Misses 237 680 +443 ☔ View full report in Codecov by Sentry. |
d71170e
to
ebf8bf3
Compare
Pull Request Test Coverage Report for Build 12885951319Details
💛 - Coveralls |
abc1f9b
to
d4c3547
Compare
I think I agree with all the changes listed in OP, although Rather than merging straight to master, should we bundle this with #2458 to form a new breaking release to go out when we feel like is the right moment? |
Happy to release both PRs together! I can't say I feel particularly bad about LogDensityFunction tbh, it's still accessible via DynamicPPL with the same functionality, so we're really just asking people to import it from where it should be imported. |
There's still the wholesale export of MCMCChains which I don't dare to touch. MCMCChains exports a lot of stuff. https://github.com/TuringLang/MCMCChains.jl/blob/master/src/MCMCChains.jl |
d4c3547
to
8fd6095
Compare
8fd6095
to
2409aa1
Compare
As mentioned in #2468, I don't think we need to export so many things. I've done a bunch of cuts here, with the intention that if we don't feel comfortable cutting something, we can easily revert specific commits. In my opinion, though, even though it looks like many sweeping changes, I don't think any of these are hugely controversial on their own.
Concretely, this PR does the following:
Stops re-exporting the DynamicPPL module.
Stops re-exporting
@logprob_str
and@prob_str
, which were deprecated several months ago (we've killed off deprecated stuff in far shorter timespans)Stops re-exporting
DynamicPPL.LogDensityFunction
, this is an implementation detail that users probably don't need to construct themselves.Bumps the Distributions.jl compat bound (from 0.23 to 0.25.77) and removes code that was required to support old versions of Distributions. Note that DynamicPPL already requires Distributions 0.25.
Removes the blanket re-export of Bijectors and Libtask, as well as the export of
Bijectors.ordered
.ordered
is used in the Gaussian mixture model tutorial, but even there it's imported directly from Bijectors, and I don't think it's unfair to expect users to manually import Bijectors if they want to use this function.Removes the blanket re-export of AbstractMCMC, replacing it with an explicit export list of
sample
,MCMCThreads
,MCMCDistributed
, andMCMCSerial
. Note that these are the only four identifiers which AbstractMCMC itself exports (https://github.com/TuringLang/AbstractMCMC.jl/blob/master/src/AbstractMCMC.jl) so the only change is that the module name itself (AbstractMCMC) is no longer accessible asTuring.AbstractMCMC
.For convenience, re-exports
LinearAlgebra.I
.Exports
DynamicPPL.returned
(apparently we never bothered to export it when deprecatinggenerated_quantities
, oops haha).Will let CI run before requesting a review.