[Merged by Bors] - Concretize IndexLens using to_indices #43
Conversation
Pull Request Test Coverage Report for Build 1485214864Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Codecov ReportBase: 80.39% // Head: 80.00% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #43 +/- ##
==========================================
- Coverage 80.39% 80.00% -0.40%
==========================================
Files 3 2 -1
Lines 255 125 -130
==========================================
- Hits 205 100 -105
+ Misses 50 25 -25
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
I added some normalization code to the index conversions, so that all ranges will become "normal ranges", as constructed by This should mitigate possible confusions in printing and problems with equality. Also, at the time lenses are actually applied for getting, the indices are converted once more using |
|
I'm generally okay with this change, but I'd like to think about it a tiny bit more. E.g.
this is quite an annoying issue. Do you have any thoughts on how we'd approach this dealing with this? |
There was a problem hiding this comment.
I guess this is quite breaking for DynamicPPL and Turing since the names of the parameters in the resulting chains etc. are based on this printed representation.
Probably even without this PR it would be good to test non-standard indices (e.g. OffsetArrays) - or is it already part of the test suite?
Sure, lets think about it. In my intuition, But my solution would be to do the same thing as the Now that's the logical answer. But if we do it this way, you compute the Line 174 in 3897231 julia> to_indices([1,2], (:,))
(Base.Slice(Base.OneTo(2)),)so we can dispatch on |
There's a major difference here though: |
|
I found that the following is also a problem, and more severe: julia> y = rand(10, 10);
julia> to_indices(x, (2:2:5,))
(2:2:4,)EDIT: OK, this isn't as severe as it seems. The construction of this slice already performs this normalization: julia> 2:2:5
2:2:4Therefore this behaviour is already happening. |
concretization flag to macro
|
I've made some changes trying to find a balance here. Namely,
I thought it would be possible to wrap the un-converted index into |
|
@torfjelde can you take a look at this PR, see whether you are happy before I merge it? |
|
I'd have to look at this one myself again; IIRC there were some unexpected subtleties. |
|
Sooo... I think we need to think about this a bit more. I have now added a couple of proper tests for @varname(A[1, Not(3)], true) == @varname(A[1, [1, 2, 4, 5, 6, 7, 8, 9, 10]])work. I have started to continue and refactor some other things (so currently many subsumption tests are broken). |
|
I got quite something, and I think it's reasonable. Please review. More test cases for non-standard indexing and general edge cases would probably be good. This assumes that non-concrete colons can never be subsumed (you cannot reasonably decide that Note that this does not apply when you conretize first; then the colon will be resolved and the comparison is done on runtime indices. |
Thanks, @phipsgabler for looking further into it. I think it is sensible to throw an error on these corner cases. It is also fine to forbid cases where it causes inconsistency with downstream packages like |
|
What remains to be discussed are the choices about colons. Is concretizing them into ranges OK, and do the subsumption rules involving them not cause troubles. I think I can argue for both (and think that some breaking changes are acceptable if it makes the overall implementation better and more consistent), but this needs to be cleared by people who can judge the downstream consequences better than me. I'll add more test cases and README documentation to lay out the behaviour later this week. |
|
It seems the PR breaks https://github.com/TuringLang/DynamicPPL.jl/blob/aeb5e03fdfe6958e4f79d9ccc5b01b84b93371ff/test/compiler.jl#L220. The model seems reasonable though, doesn't it? |
Co-authored-by: David Widmann <[email protected]>
It does indeed. I need to take a look at that. |
|
@phipsgabler a gentle reminder |
|
bors try |
tryMerge conflict. |
|
bors try |
tryBuild failed: |
|
The remaining DPPL errors are, I think, simply caused by the fact that |
|
If it breaks DynamicPPL, I guess it should be put into a breaking release? Edit: Then also downstream tests would pass (they are canceled and pass if the downstream package is not compatible yet). |
phipsgabler
dismissed
torfjelde’s stale review
I don't see why this is still here... am I missing something?
|
bors r+ |
Should implement #35. The "problem" with this is that the resulting values are not exactly what you'd write by hand: ```julia julia> AbstractPPL.concretize(@varname(x.a[1:end, end][:]), x) x.a[1:2,2][Base.Slice(Base.OneTo(2))] ``` That wouldn't be so much of a problem besides printing, but unfortunately, lenses currenctly compare equality using strict types: ```julia julia> AbstractPPL.concretize(@varname(x[1:3, :]), rand(10, 10)).lens.indices == @varname(x[1:3, 1:10]).lens.indices true julia> AbstractPPL.concretize(@varname(x[1:3, :]), rand(10, 10)).lens == @varname(x[1:3, 1:10]).lens false julia> AbstractPPL.concretize(@varname(x[1:3, :]), rand(10, 10)) == @varname(x[1:3, 1:10]) false ``` Cf. jw3126/Setfield.jl#165; the equality comparison can hopefully be fixed there. The remaining thing is that subsumption must still be able to work with `Colon`, since a user might index a trace/VarInfo using a non-concretized varname containing a colon. But at least we can then be sure that one side is always concrete. Co-authored-by: Hong Ge <[email protected]> Co-authored-by: Hong Ge <[email protected]>
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
| if Meta.isexpr(expr, :ref) || Meta.isexpr(expr, :.) | ||
| # Split into object/base symbol and lens. | ||
| sym_escaped, lens = Setfield.parse_obj_lens(expr) | ||
| # Setfield.jl escapes the return symbol, so we need to unescape | ||
| # to call `QuoteNode` on it. | ||
| sym = drop_escape(sym_escaped) | ||
|
|
||
| return if Setfield.need_dynamic_lens(expr) | ||
| :( | ||
| if concretize |
There was a problem hiding this comment.
@phipsgabler Do you remember why this change was made?
There was a problem hiding this comment.
In particular, why it was made false by default.
Should implement #35. The "problem" with this is that the resulting values are not exactly what you'd write by hand:
That wouldn't be so much of a problem besides printing, but unfortunately, lenses currenctly compare equality using strict types:
Cf. jw3126/Setfield.jl#165; the equality comparison can hopefully be fixed there.
The remaining thing is that subsumption must still be able to work with
Colon, since a user might index a trace/VarInfo using a non-concretized varname containing a colon. But at least we can then be sure that one side is always concrete.