cat: preserve lookup values by demoting order to Unordered on misalignment#1208
cat: preserve lookup values by demoting order to Unordered on misalignment#1208ghyatzo wants to merge 3 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1208 +/- ##
==========================================
+ Coverage 80.15% 80.16% +0.01%
==========================================
Files 61 61
Lines 5851 5890 +39
==========================================
+ Hits 4690 4722 +32
- Misses 1161 1168 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
test failures are unrelated (Makie) and I have all greens on 1.12.6 locally |
691c5d3 to
ddede9b
Compare
…ned concatenation
Changes check_cat_lookups to return a Symbol tri-state (:ok/:demote/:error/)
instead of Bool, so that the _cat path can distinguish between:
- :ok — contiguous ordered lookups → proceed normally
- :demote — misaligned/mixed-order lookups with no exact value overlap
→ preserve concatenated values with order=Unordered()
- :error — exact value overlap across lookups → throw
- :drop — span/step mismatch → warn+NoLookup (unchanged behaviour)
A new _vcat_dims_demote helper concatenates the index values (via the
existing _vcat_index machinery) and rebuilds the dimension with
order=Unordered() and an Irregular span. The hcat/vcat/vcat(Dimension)
paths are updated to handle the Symbol return but retain their existing
warn+fallback-to-Base behaviour.
Overlap detection is deferred to the misaligned branch only, so the
happy (contiguous) path pays zero allocation overhead — identical
performance to the original Bool-based code.
Fixes the core complaint in rafaqz#1207: cat no longer silently drops lookup
values for ordered-but-not-strictly-contiguous inputs. Overlap still
errors via DimensionMismatch.
ddede9b to
c0e3e67
Compare
|
pushed a fix to recover a pretty bad regression in perfomance on the happy path. I was computing the intersection set too eagerly. Now it's comparable to master. this PR (before): |
rafaqz
left a comment
There was a problem hiding this comment.
This is a really nice PR. We never put the time in to properly resolve these Unordered lookups properly before but its correct.
My main comment is that the status symbols could more clearly communicate the status or action it triggers.
|
I am wondering whether we should rather return the order types from check_cat_lookups instead of symbols? |
|
I think the symbols specify more than just the order, like Nolookup is |
|
what about this for the status
it separates more the "what" from the "what we should do about it" which now are indeed a bit intertwined. EDIT: went with |
6387ca2 to
8b85b6d
Compare
8b85b6d to
4ece4f1
Compare
Hello,
Currently,
cat(orBase.cat) of twoAbstractDimArrays (orAbstractDimStacks) along a dimension whose lookups areForwardOrderedbut not strictly contiguous silently drops the lookup values, replacing them withNoLookup(). For example:Once the coordinates are gone, there is no way to sort/recover them
sortby(orBase.sort) also drops lookups (differently, but just as fatally). This makes it impossible to concatenate disjoint time-series chunks and then sort the result.Closes #1207
Approach
Replace the
check_cat_lookups(...)::Boolreturn with a Symbol tri-state:_cat:okNoLookup_vcat_dims):demote_vcat_dims_demote, order →Unordered(), span →Irregular:errorintersect(lookups...)non-empty)throw(DimensionMismatch(...)):droprebuild(catdim, NoLookup())(unchanged)The new
_vcat_dims_demotehelper reuses the existing_vcat_indexmachinery (the samemaybeshiftlocus+reduce(vcat, …)from the success path) to concatenate the raw lookup values, then rebuilds the dimension withorder=Unordered()and anIrregularspan. This keeps the coordinates intact so downstream operations likesortperm+ indexing (or a futuresortby) can recover aForwardOrderedresult.For the fallbacks
hcat/vcat/vcat(::Dimension,…)the Symbol return is handled but behaviour is unchanged: they still warn and fall back toBase.hcat(map(parent,…))/Base.vcat(map(parent,…)).Example after the fix
The values are preserved. A subsequent
sortby(ords[Ti=sortperm(lookup(ds,Ti))])recovers
ForwardOrdered.What changed
src/array/methods.jlcheck_cat_lookups→ returns::Symbolinstead of::Bool_check_cat_lookup_order→ returns:ok/:demote/:error; tracks anaccumulated
Setto detect exact value overlap_check_cat_lookups→ two-level chain: order check passes through to spancheck only on
:ok;:demoteshort-circuits (span validation is meaninglesswhen demoted)
_check_cat_lookups(D, ::Regular, …)/::Explicit/::Irregular→ return:ok/:drop(converted fromtrue/false)_vcat_dims_demote(d1, ds…)— concatenates values and rebuilds withorder=Unordered(),span=Irregular(combined bounds)forIntervals/Irregular(nothing,nothing)forPoints_catcall site: branches on:ok/:demote/:error/else(:drop)hcat,vcat,vcat(::Dimension)call sites: only:oksucceeds; anythingelse falls back (unchanged behaviour)
Unorderdtypo →Unorderedin_cat_lookup_intersect_warntest/methods.jl@test_warn "misaligned" cat(da, db; dims=2)→@test_throws(identical Y lookups = exact overlap, now correctly errors)
"misaligned ordered lookups are demoted to Unordered"testset(reproduces
catdrops lookups, no avenues for sorting? #1207, assertsUnordered+ values preserved)"overlap throws DimensionMismatch via cat"testsetOpen questions
hcat/vcatinconsistency: should they also demote on misalignmentrather than falling back to a plain Array? Currently out of scope — only
_catwas changed. Maybe a separate PR?sortbycompanion: a helper likesortby(x, dim) = set(x[rebuild(dims(x,dim), sortperm(lookup(x,dim)))], dim => ForwardOrdered())is a natural partner to this fix. Should it be added in a follow-up PR
or included here?
DimensionMismatchin_cat. Forhcat/vcatit still warns andfalls back. Should this be made consistent in a follow-up?
Related:
catfor rasterstacks #1014 touches the same code but it's orthogonal to this one, both can be merged together mechanically.Disclosure
PR was an initial hand implementation of #1207 (comment), then checked and refined with an agent.