Reorganise code in src/mcmc/Inference.jl#2573
Conversation
|
Turing.jl documentation for PR #2573 is available at: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2573 +/- ##
=======================================
Coverage 85.86% 85.86%
=======================================
Files 21 24 +3
Lines 1429 1429
=======================================
Hits 1227 1227
Misses 202 202 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Pull Request Test Coverage Report for Build 15348678165Details
💛 - Coveralls |
mhauru
left a comment
mhauru
left a comment
There was a problem hiding this comment.
Makes sense to me, just a few questions.
| @@ -0,0 +1,92 @@ | |||
| module Sample | |||
There was a problem hiding this comment.
Any particular reason to have this as a submodule but the others not? Also, do we have a convention that files that define a module have their names CamelCased?
There was a problem hiding this comment.
Any particular reason to have this as a submodule but the others not?
I would like everything to be a submodule, but I thought changing it now would be unnecessarily destructive.
Also, do we have a convention that files that define a module have their names CamelCased?
As in, this should be src/mcmc/Sample.jl? I'm not sure I recall hearing about such a convention.
There was a problem hiding this comment.
I'm not sure it's documented anywhere, but I've noticed a correlation between things that are submodules and things that are CamesCased (as in src/mcmc/Sample.jl). I wonder if there are any Julia/community guidelines for this.
There was a problem hiding this comment.
I suggest avoiding a module here, since it mostly overloads AbstractMCMC.sample functions.
Let's use a submodule only for a coherent set of types and APIs. I appreciate that this might differ from other languages, but staying close to Julia's practices is better.
There was a problem hiding this comment.
We can merge sample.jl into the existing file abstratmcmc.jl .
There was a problem hiding this comment.
Yes, although it's not just about what I'm used to from a different language -- the main benefit of having code organised in modules is to prevent imports from leaking into / out of files. We discussed this previously here #2288 -- and I still believe it's a good thing to put code into modules, and my opinion is independent of what the Julia community or any other language's community does.
Of course, maybe it's overkill to create a module just for these methods, since by definition it's not possible to leak code out of new method overloads.
There was a problem hiding this comment.
And in fact, abstractmcmc.jl should be put in external_sampler.jl, since the bulk of its code deals with ExternalSampler.
(Specifically, it implements the AbstractMCMC interface for ExternalSampler, and thus it should be in the same code file as the definition of ExternalSampler --- just like how the implementation of the AbstractMCMC interface for NUTS is in hmc.jl.)
There was a problem hiding this comment.
Removed the module now, so I assume this settles the question of file naming (I did have a very, very brief look on Julia Discourse, and couldn't find anything).
penelopeysm
force-pushed
the
py/reorganise-inference
branch
from
13b14ac to
30bd7a5
Compare
|
Will merge now, but since this is a no-op in terms of code changes, happy to address any followup issues in a new PR |
github-merge-queue
Bot
removed this pull request from the merge queue due to no response for status checks
* Reorganise code in src/mcmc/Inference * Don't make src/mcmc/sample a submodule * Move abstractmcmc.jl to external_sampler.jl
4 participants
This PR does some code reshuffling in
src/mcmc/Inference.jl.In particular:
sample()dispatch methods are moved tosample.jlPrior()is moved to its own file,prior.jl(it's an inference algorithm after all)ExternalSampleris moved to its own file,external_sampler.jl. The existing implementation of AbstractMCMC methods on ExternalSampler, which used to beabstractmcmc.jl, has been moved into this file as well, so that the naming is more coherent.InferenceAlgorithmis moved to its own file,algorithm.jl. This might seem excessive, as it's just one abstract type. However, as part of the work on Reworksample()call stack to use LogDensityFunction #2555 I am going to introduce a few interface functions that InferenceAlgorithms should implement. Note that we actually already have some of these, such asrequires_unconstrained_spaceorgetADtype, that are scattered around this file / implemented for some samplers but not all. I think it would be good to unify these, and my eventual vision is thatalgorithm.jlwill be where the interface + default implementations live. Some of this work is already in Rework sample() call stack, part 1: HMC #2566.The remainder is still quite messy, but it can be for another day...