-
Notifications
You must be signed in to change notification settings - Fork 35
Remove 3-argument {_,}evaluate!!
; clean up submodel code
#960
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
Open
penelopeysm
wants to merge
5
commits into
breaking
Choose a base branch
from
py/no-context-evaluate-submodel
base: breaking
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
_evaluate!!
; clean up submodel code{_,}evaluate!!
; clean up submodel code
3dc22f4
to
4bb2526
Compare
Benchmark Report for Commit bfa48b3Computer Information
Benchmark Results
|
4bb2526
to
55f838f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## breaking #960 +/- ##
============================================
- Coverage 82.85% 82.76% -0.09%
============================================
Files 38 38
Lines 4031 4010 -21
============================================
- Hits 3340 3319 -21
Misses 691 691 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dcd5c9d
to
31b0caa
Compare
DynamicPPL.jl documentation for PR #960 is available at: |
31b0caa
to
039a523
Compare
039a523
to
bfa48b3
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR:
evaluate!!(model, varinfo, context)
in the DynamicPPL codebase_evaluate!!(model, varinfo, context)
as this method was only being used for submodelsIn return, it introduces:
_evaluate!!(submodel, varinfo, context, left)
. Yes, yes, I basically moved the implementation from one place to the other; but the whole point is that this was special-case behaviour that was only needed for submodels, so this at least makes it clearer.It also removes a lot of wrapper code for submodels. I personally don't see the need for more than one layer of indirection for submodels (in terms of the data structure, the only difference between a submodel and a model is that the submodel carries information about whether it should be auto-prefixed). I had mucked around with this in #815 and I didn't find any real problems with removing the wrappers there (in fact in that PR I went one step further and removed all wrappers, but that would have made it impossible to opt-out of prefixing and that's Bad(TM)).
I didn't add any new tests, but
test/submodel.jl
already contains quite extensive tests to make sure that everything behaves and it passes these tests so I'm quite happy.#959 needs to be merged first.Done.Follow-up from #952
Closes #720 (for good)