Breaking: Include refdims as columns in DimTable

[sethaxen]

This PR adds a keyword refdims to DimTable, defaulting to the refdims of the input. All refdims are included as table columns. Fixes #884

Potentially breaking changes:

• dims field added to DimTable
• refdims columns included by default

Example

julia> using DimensionalData, Tables

julia> using DimensionalData, Tables

julia> ds = DimStack((; x=DimArray(ones(5, 3), (X, Y)), y=DimArray(zeros(5), X)))  # current behavior unchanged
┌ 5×3 DimStack ┐
├──────────────┴───────────────────── dims ┐
  ↓ X, → Y
├────────────────────────────────── layers ┤
  :x eltype: Float64 dims: X, Y size: 5×3
  :y eltype: Float64 dims: X size: 5
└──────────────────────────────────────────┘

julia> Tables.columntable(DimTable(ds))  # without refdims, current behavior unchanged
(X = [1, 2, 3, 4, 5, 1, 2, 3, 4, 5, 1, 2, 3, 4, 5], Y = [1, 1, 1, 1, 1, 2, 2, 2, 2, 2, 3, 3, 3, 3, 3], x = [1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0], y = [0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0])

julia> Tables.columntable(DimTable(ds[Y(3)]))  # refdims included by default
(X = [1, 2, 3, 4, 5], Y = [3, 3, 3, 3, 3], x = [1.0, 1.0, 1.0, 1.0, 1.0], y = [0.0, 0.0, 0.0, 0.0, 0.0])

julia> Tables.columntable(DimTable(ds[Y(3)]; refdims=()))  # manually opt out of refdims being included
(X = Base.OneTo(5), x = [1.0, 1.0, 1.0, 1.0, 1.0], y = [0.0, 0.0, 0.0, 0.0, 0.0])

julia> Tables.columntable(DimTable(ds; refdims=(Z(1:2),)))  # manually add new refdims (all columns repeat)
(X = [1, 2, 3, 4, 5, 1, 2, 3, 4, 5  …  1, 2, 3, 4, 5, 1, 2, 3, 4, 5], Y = [1, 1, 1, 1, 1, 2, 2, 2, 2, 2  …  2, 2, 2, 2, 2, 3, 3, 3, 3, 3], Z = [1, 1, 1, 1, 1, 1, 1, 1, 1, 1  …  2, 2, 2, 2, 2, 2, 2, 2, 2, 2], x = [1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0  …  1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0], y = [0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0  …  0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0])

[sethaxen]

It's not clear from the documentation what assumptions this method makes about xs, so it's possible there's a mistake in this method.

[sethaxen]

Is it at all weird that dims(t::DimTable) will return fewer dims than the actual dims included in the table (because it just forwards to the parent)?

[rafaqz]

I didn't even know it did that. We can fix these things and merge to breaking instead?

[sethaxen]

It didn't do that until this PR, since previously the table's dims were the parent dims, but now additional dims may be included.

[rafaqz]

Ahh you mean the refdims? Yeah hkw do we keep that separate.

[sethaxen]

2 ways I can think of:

• Keep dims(::DimTable) forwarding to the parent and add the method refdims(t::DimTable) = otherdims(dims(t), _dims(t)). Maybe not the right way to go if the parent is a slice of a DimMatrix where both dimensions have the same dim, but that kind of thing in general may not be well supported.
• Remove the dims field and add a refdims and refdimarrays field. Then refdims(t::DimTable) = getfield(t, :refdims). I started implementing this version originally and abandoned it because it makes the column indexing more complicated, but I could bring it back.

[rafaqz]

Can we not just do refdims(dt::DimTable) = refdims(parent(dt)) ? Maybe I'm missing something

[sethaxen]

I guess it depends on what dims(::DimTable) and refdims(::DimTable) should mean. With this PR, a user can provide arbitrary refdims to the DimTable, so they may not even be in the parent. But should refdims(::DimTable) return those user-provided refdims (currently only stored in (::DimTable).dims) or return those of the parent? Should dims(::DimTable) return just the dims of the parent, or should it return all dimensions corresponding to columns (with this PR, also includes user-provided refdims).

[sethaxen]

Yeah, it's kind of weird that we need getfield(::DimTable, :dims) calls everywhere. I think I'd like to change it so that dims(::DimTable) is the one accessor that doesn't forward to the parent.

[codecov]

Codecov Report

❌ Patch coverage is 96.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.78%. Comparing base (2e5a812) to head (67d3f0f).
⚠️ Report is 7 commits behind head on breaking.

Files with missing lines	Patch %	Lines
ext/DimensionalDataAlgebraOfGraphicsExt.jl	66.66%	1 Missing ⚠️
src/tables.jl	97.87%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           breaking    #1119      +/-   ##
============================================
+ Coverage     82.43%   82.78%   +0.34%     
============================================
  Files            57       57              
  Lines          5647     5668      +21     
============================================
+ Hits           4655     4692      +37     
+ Misses          992      976      -16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tiemvanderdeure]

I think I would prefer if this was a bool that determines whether or not refdims are included as columns

[sethaxen]

Makes sense, though the current approach would support only including a subset of refdims, which maybe is useful in some cases? What other methods implemented here produce refdims besides slicing?

[rafaqz]

I don't mind the current syntax with a tuple as it's the same as the constructor syntax, and its flexible.

But to merge this to main () would need to be the default.

We can then change that on breaking.

[rafaqz]

@sethaxen CF standards has its own refdims concept so Rasters.jl Raster can load with refdims already in place.

[rafaqz]

@sethaxen the conflicts against breaking are pretty extreme, sorry about that.

But we should try getting it merged there for the next breaking release (which is very soon)

[sethaxen]

@sethaxen the conflicts against breaking are pretty extreme, sorry about that.

But we should try getting it merged there for the next breaking release (which is very soon)

Currently the default behavior is to not add refdims to columns. What makes this breaking? And, if this will be considered breaking anyways, should I then restore the default behavior being to include refdims in columns?

[rafaqz]

Oh right I misunderstood. So yes its fine to merge to main.

But: then there will be the other problem that I cant easily do the merge because neither side of the Tables.jl changes are my code, and its a lot.

If I merge to main can you commit to merging main into breaking soon afterwards? I will make sure its only your changes to merge.

[sethaxen]

Oh right I misunderstood. So yes its fine to merge to main.

But: then there will be the other problem that I cant easily do the merge because neither side of the Tables.jl changes are my code, and its a lot.

If I merge to main can you commit to merging main into breaking soon afterwards? I will make sure its only your changes to merge.

Oh I see what you mean. Oof, both PRs rewrite the tables tests. But most of the changes to tests in this PR are I think it will actually be easier to redo this PR onto breaking. Okay with me opening a new PR against breaking with these changes (with default being to include refdims as columns)?

[rafaqz]

Yes changing the default would be fine on breaking.

But this PR is already moved to breaking so no need for a new one.