Skip to content

Conversation

@felixcremer
Copy link
Collaborator

This is to make sure, that DiskArrays don't getindex into data during show.
I added a fallback to true so that for non-DiskArray datasets this would not change in behaviour.

I thought about using isdisk, but I wasn't sure, where I could add this check because we don't have isdisk available in DimensionalData and in the extension it is too late.

I also added a test, that checks that DiskArrays based DimArrays don't access the data.

Copy link
Owner

@rafaqz rafaqz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can_show_data is a bit clearer

@felixcremer felixcremer requested a review from rafaqz November 4, 2025 15:37
@felixcremer
Copy link
Collaborator Author

I applied your suggestions and fixed the show test.
Locally I get the following unrelated test failure n the "inference" testset:

julia> x = DimArray(randn(2, 3, 4), (X, Y, Z));

julia> foo(x) = maximum(x; dims=(1, 2))
foo (generic function with 1 method)

julia> @inferred foo(x)
ERROR: return type DimArray{Float64, 3, Tuple{X{DimensionalData.Dimensions.Lookups.NoLookup{Base.OneTo{Int64}}}, Y{DimensionalData.Dimensions.Lookups.NoLookup{Base.OneTo{Int64}}}, Z{DimensionalData.Dimensions.Lookups.NoLookup{Base.OneTo{Int64}}}}, Tuple{}, Array{Float64, 3}, DimensionalData.NoName, DimensionalData.Dimensions.Lookups.NoMetadata} does not match inferred return type DimArray{Float64, 3, D, Tuple{}, Array{Float64, 3}, DimensionalData.NoName, DimensionalData.Dimensions.Lookups.NoMetadata} where D<:Tuple
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:44
 [2] top-level scope
   @ REPL[30]:1

I also get this on DimensionalData 0.29.24

@codecov
Copy link

codecov bot commented Nov 4, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.91%. Comparing base (6db30de) to head (527e653).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1130   +/-   ##
=======================================
  Coverage   86.90%   86.91%           
=======================================
  Files          55       55           
  Lines        5338     5341    +3     
=======================================
+ Hits         4639     4642    +3     
  Misses        699      699           

☔ 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.

This is to fix the nightly tests. Because the behaviour has changed in Julia 1.14
@felixcremer
Copy link
Collaborator Author

The last commit was needed because the string before a multiple instance assignment fails now after JuliaLang/julia#59742. I don't see why we wanted to have a string there anyway so I converted it to proper comments.

@rafaqz rafaqz merged commit 6e33f9d into rafaqz:main Nov 5, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants