-
Notifications
You must be signed in to change notification settings - Fork 9
VarName rework #150
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
base: main
Are you sure you want to change the base?
VarName rework #150
Conversation
|
AbstractPPL.jl documentation for PR #150 is available at: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #150 +/- ##
==========================================
- Coverage 86.40% 84.32% -2.08%
==========================================
Files 9 10 +1
Lines 456 555 +99
==========================================
+ Hits 394 468 +74
- Misses 62 87 +25 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4a2ab9f to
0282e15
Compare
|
One very high-level comment—without having looked closely at the code changes—is that support for |
sunxd3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heroic effort!
some minor issues, in general, this is definitely the right way to go
|
@yebai: I don't think what you're describing is an issue with indexing with |
Co-authored-by: Xianda Sun <[email protected]>
| The idea of concretization is not new to AbstractPPL. | ||
| However, there are some differences: | ||
|
|
||
| - Colons are no longer concretized: they *always* remain as Colons, even after calling `concretize`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? Doesn't this undermine the purpose of concretisation? I understand the comment in the DPPL PR of eventually getting rid of concretisation in DPPL, but this doesn't seem like the solution to that.
| The `Base.get` and `Accessors.set` methods for VarNames have been removed (these were responsible for method ambiguities). | ||
| Instead of using these methods you can first convert the `VarName` to an optic using `varname_to_optic(vn)`, and then use the getter and setter methods on the optics. | ||
|
|
||
| VarNames cannot be composed with optics now (compose the optics yourself). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I still prefix an optic with a VarName? That feels like it would be handy. I agree that compose is not the right term for it.
|
I was a bit overly terse there: I'm generally a fan of this. I didn't read most of the code, but I like the idea of what is being done here. Just had the above two questions about interface changes. |
Closes #49 (we can't always use views but this PR does so where possible; see the
_maybe_viewfunction for details)Closes #136 (by removing the methods)
Closes #148 (note that we can't fully remove the dependency on Accessors.jl, not without reimplementing a ton of stuff, but we have removed the dependency on its types)
This PR modifies
VarNameto use in-house optics, instead of the optics from Accessors.jl. See the HISTORY.md for details. This PR also adds proper documentation, which you can read at https://turinglang.org/AbstractPPL.jl/previews/PR150/varname/.I'm aware that this is practically impossible to review, but if you are seeing this, you can take heart from the fact that pretty much all the pre-existing tests for high-level functions like
prefix,unprefix,varname_leaves,hasvalue,getvalue, have not been touched, and continue to pass. The only cases where I had to change the tests were those where the actual semantics were changed, e.g.beginis now no longer automatically concretised. (In fact, this PR also expands the test suite by quite a bit.) Besides, DynamicPPL CI passes just fine with only a handful of interface changes, and benchmarks don't show any performance regressions: TuringLang/DynamicPPL.jl#1185Essentially, I think it's fair to say that once you have constructed a
@varname(...)it will pretty much behave the same way it used to, for the most part you just need to change the types soIdeninstead oftypeof(identity), etc.Of course, this is no longer true if you are digging into
getoptic(vn)and things like that. The data structure changes are all insidesrc/varname/optic.jlandsrc/varname/varname.jl. It looks complicated, but that's because I have made it very general: it's more general than old VarName used to be (begin/endno longer need to be concretised so early), and also more general thanAccessors.IndexLensis (it accepts keyword arguments). The reason for this is because, if we are undertaking a big refactoring, we may as well do it correctly. Otherwise in the future if we want Turing to work with other array types (looking atDimArrayin particular) we will have to come back and fix it again.Note that keyword argument support for
getindex/setindex!can be a bit patchy, but:varname_and_value_leavesmight need patches to make it work nicely, which I haven't implemented.BangBang.setindex!!will error if you try to use keyword arguments (so in here I've defaulted to usingBase.setindex!). In fact, if anything, I think it's good that we find cases like these to push the ecosystem forward.