rafaqz / DimensionalData.jl

Named dimensions and indexing for julia arrays and other data
https://rafaqz.github.io/DimensionalData.jl/stable/
MIT License
262 stars 38 forks source link

[WIP] Use DataAPI.metadata instead of redefining it #707

Open asinghvi17 opened 2 months ago

asinghvi17 commented 2 months ago

Fix #706 by explicitly importing DataAPI.metadata.

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 83.87%. Comparing base (37cd1eb) to head (05c464d). Report is 3 commits behind head on main.

:exclamation: Current head 05c464d differs from pull request most recent head a10403b. Consider uploading reports for the commit a10403b to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #707 +/- ## ========================================== + Coverage 83.86% 83.87% +0.01% ========================================== Files 46 46 Lines 4257 4267 +10 ========================================== + Hits 3570 3579 +9 - Misses 687 688 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

rafaqz commented 2 months ago

May need to check we don't have function metadata end or metadata(x) = nothing lying around

rafaqz commented 1 month ago

We have function metadata end in interface.jl that needs removing.

And these methods are going to be a piracy problem:

  [2] metadata(::Tuple{})
     @ DimensionalData.Dimensions ~/.julia/dev/DimensionalData/src/Dimensions/dimension.jl:232
 [15] metadata(ds::Tuple, I)
     @ DimensionalData.Dimensions ~/.julia/dev/DimensionalData/src/Dimensions/dimension.jl:234
 [16] metadata(ds::Tuple, i1, I...)
     @ DimensionalData.Dimensions ~/.julia/dev/DimensionalData/src/Dimensions/dimension.jl:233
 [17] metadata(ds::Tuple)
     @ DimensionalData.Dimensions ~/.julia/dev/DimensionalData/src/Dimensions/dimension.jl:231
 [18] metadata(x)
     @ ~/.julia/dev/DimensionalData/src/Lookups/metadata.jl:107

I guess we can make all the Tuples into DimTuple so we own them, with the slight downside that it will error on empty Tuple{}.

The untyped case may be a problem too, we can try deleting it and see what breaks.

asinghvi17 commented 1 month ago

Thanks - will take a look today!

asinghvi17 commented 1 month ago
julia> DataAPI.metadata(tuple())
ERROR: ArgumentError: Objects of type Tuple{} do not support reading metadata
Stacktrace:
 [1] metadata(x::Tuple{}; style::Bool)
   @ DataAPI ~/.julia/packages/DataAPI/atdEM/src/DataAPI.jl:373
 [2] metadata(x::Tuple{})
   @ DataAPI ~/.julia/packages/DataAPI/atdEM/src/DataAPI.jl:371
 [3] top-level scope
   @ REPL[41]:1

so that method is at least open...

asinghvi17 commented 1 month ago

Ah damn, I just realized that the way that DD handles metadata (DD.NoLookup) is not the same way as DataAPI handles metadata (Nothing). I guess this could potentially stay the same for now, and maybe just hook into DataAPI.metadata as well for DD native types...

rafaqz commented 1 month ago

DD.NoMetadata but we could maybe change it. One nice thing about NoMetadata is Dict functions like haskey work on it (and we can as more as needed) so its a nice fallback that both wont cause errors anywhere and e.g. works on GPUs like nothing would.