-
Notifications
You must be signed in to change notification settings - Fork 32
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
Remove selector stuff from VarInfo tests and link/invlink #780
Merged
+513
−501
Merged
Changes from 18 commits
Commits
Show all changes
33 commits
Select commit
Hold shift + click to select a range
4dc2a72
Remove selector stuff from varinfo tests
mhauru 9b492a3
Implement link and invlink for varnames rather than samplers
mhauru b508f08
Replace set_retained_vns_del_by_spl! with set_retained_vns_del!
mhauru b8880d1
Make linking tests more extensive
mhauru 99a8490
Remove sampler indexing from link methods (but not invlink)
mhauru 4a79b1f
Remove indexing by samplers from invlink
mhauru 26a1901
Merge remote-tracking branch 'origin/master' into mhauru/remove-selec…
mhauru 090608b
Work towards removing sampler indexing with StaticTransformation
mhauru 4749853
Fix invlink/link for TypedVarInfo and StaticTransformation
mhauru e960679
Fix a test in models.jl
mhauru d507a53
Move some functions to utils.jl, add tests and docstrings
mhauru 41150b5
Fix a docstring typo
mhauru 836fb13
Merge branch 'release-0.35' into mhauru/remove-selectors-linking
mhauru 45d1f13
Various simplification to link/invlink
mhauru 98915c2
Improve a docstring
mhauru f05068d
Style improvements
mhauru bc4c420
Fix broken link/invlink dispatch cascade for VectorVarInfo
mhauru 71980ba
Fix some more broken dispatch cascades
mhauru 45562a9
Apply suggestions from code review
mhauru db5b835
Remove comments that messed with docstrings
mhauru f99effe
Apply suggestions from code review
mhauru 56194cd
Fix issues surfaced in code review
mhauru c187c49
Simplify link/invlink arguments
mhauru 86b25c5
Fix a bug in unflatten VarNamedVector
mhauru 2a6c1bc
Rename VarNameCollection -> VarNameTuple
mhauru 853f47e
Remove test of a removed varname_namedtuple method
mhauru ed80328
Apply suggestions from code review
mhauru d996d0c
Respond to review feedback
mhauru 2083148
Remove _default_sampler and a dead argument of maybe_invlink_before_eval
mhauru 39fa647
Fix a typo in a comment
mhauru 9df364f
Merge remote-tracking branch 'origin/release-0.35' into mhauru/remove…
mhauru 2c73de5
Add HISTORY entry, fix one set_retained_vns_del! method
mhauru 49604e1
Merge remote-tracking branch 'origin/release-0.35' into mhauru/remove…
mhauru File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Oops, something went wrong.
Oops, something went wrong.
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.
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.
I mightttt have mentioned this before, but I still would really prefer if we didn't define a single-VarName method.
vn
which might be a VarName or an iterable of VarNames depending on the context, and it's difficult to guess which one is happening when it gets passed to a function that can take either.AbstractTransformation
and N types ofAbstractVarInfo
, then for every possible type that we let thevns
parameter be, we are essentially declaring M*N more methods, for which we need to make sure that they all exist, behave correctly, and don't generate any method ambiguities (the act of resolving method ambiguities itself leads to code duplication).(But at least it's better than allowing a new optional parameter, for which complexity grows exponentially, cf. our situation with
evaluate!!
.)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.
I've removed this now, even though I would like to have the single-VarName version. The reason I've removed it is that the dispatch on
link
/invlink
is already too complicated, which indeed causes method ambiguities. I think if the dispatch got cleaned up otherwise we could reintroduce the single-VarName version, which we should be able to do with just a single method without ambiguities (because no other method would accept a single VarName). However, for now it's not worth the code complexity, given that it's not really needed anywhere.For the first point of being able to see the type at the call site, I see what you mean, but I think this is something you have to largely give up in a dynamically typed language like Julia, and I think the ergonomy of the single-VarName method would outweigh it.
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.
Maybe not very surprising, but I don't agree with this 🙈
Dynamic typing means that it's not possible to unambiguously determine the type of something (using type inference algorithms, or in practice, LSP), but often it's possible to make limited inferences from the known function signatures. In a dynamically typed language, it's not possible to verify these inferences at compile time, but IMO it doesn't follow from this that we should make it harder for ourselves to make such inferences.
The most frequently cited benefit of dynamic typing is the ease of prototyping. However, I think this primarily applies to the end users of the language, i.e. people who end up using Turing. If this was a case of providing nice APIs for end users, I'd be totally on board with extra convenience methods, but I think the
(inv)?link(!!)?
functions are pretty much entirely for internal use. I believe (and I hope it's not a controversial opinion!) that it's easier to maintain, reason about, and guarantee the correctness of software that is statically typed, and although we can't do that in Julia, I do think that any step we can take in that direction is a net positive, and our lives will be easier if we are more disciplined about our internal APIs 😉Anyway, I just wanted to write it out because I suspect that me pushing back against multiple dispatch 'overuse' (quotes because others may not agree that it's overuse!) might be a continuing trend in PRs. I do recognise it's not my personal codebase and I'll always be happy to compromise on the outcome, in the sense that if you say you really want a convenience method I'll just say 'ehh, whatever', but I think it's unlikely that I'll really change my mind on the underlying principle. So I write it out once and I'll spare all of you this next time 😄 Maybe I will bookmark it and copy paste the link whenever people ask me about it hahah
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.
(Btw just on this PR I'm happy with how it stands with dispatch so feel free to resolve this convo!)