Skip to content

Stricter BlockRange constructors #472

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

mtfishman
Copy link
Collaborator

@mtfishman mtfishman commented Jun 30, 2025

Closes #471.

This removes constructors BlockRange(), BlockRange(2, 2), and BlockRange(1:2, 1:2). Instead, the more explicit versions BlockRange(()), BlockRange((2, 2)), and BlockRange((1:2, 1:2)) can be used.

This now makes BlockRange constructors more consistent with CartesianIndices constructors:

julia> CartesianIndices()
ERROR: MethodError: no method matching CartesianIndices()
The type `CartesianIndices` exists, but no method is defined for this combination of argument types when trying to construct it.

Closest candidates are:
  CartesianIndices(::Tuple{})
   @ Base multidimensional.jl:271
  CartesianIndices(::AbstractArray)
   @ Base multidimensional.jl:281
  CartesianIndices(::R) where {N, R<:NTuple{N, OrdinalRange{Int64, Int64}}}
   @ Base multidimensional.jl:268
  ...

Stacktrace:
 [1] top-level scope
   @ REPL[9]:1
 [2] top-level scope
   @ none:1

julia> CartesianIndices(2, 2)
ERROR: MethodError: no method matching CartesianIndices(::Int64, ::Int64)
The type `CartesianIndices` exists, but no method is defined for this combination of argument types when trying to construct it.

Closest candidates are:
  CartesianIndices(::Tuple{})
   @ Base multidimensional.jl:271
  CartesianIndices(::AbstractArray)
   @ Base multidimensional.jl:281
  CartesianIndices(::R) where {N, R<:NTuple{N, OrdinalRange{Int64, Int64}}}
   @ Base multidimensional.jl:268
  ...

Stacktrace:
 [1] top-level scope
   @ REPL[12]:1
 [2] top-level scope
   @ none:1

julia> CartesianIndices(1:2, 1:2)
ERROR: MethodError: no method matching CartesianIndices(::UnitRange{Int64}, ::UnitRange{Int64})
The type `CartesianIndices` exists, but no method is defined for this combination of argument types when trying to construct it.

Closest candidates are:
  CartesianIndices(::AbstractArray)
   @ Base multidimensional.jl:281
  CartesianIndices(::Tuple{})
   @ Base multidimensional.jl:271
  CartesianIndices(::R) where {N, R<:NTuple{N, OrdinalRange{Int64, Int64}}}
   @ Base multidimensional.jl:268
  ...

Stacktrace:
 [1] top-level scope
   @ REPL[13]:1
 [2] top-level scope
   @ none:1

julia> CartesianIndices(())
CartesianIndices(())

julia> CartesianIndices((2, 2))
CartesianIndices((2, 2))

julia> CartesianIndices((1:2, 1:2))
CartesianIndices((1:2, 1:2))

and removes the inconsistency pointed out in #471 when calling BlockRange(::AbstractUnitRange) vs. BlockRange(::AbstractVector):

julia> using BlockArrays

julia> collect(BlockRange(blockedrange([2, 3])))
5-element Vector{Block{1, Int64}}:
 Block(1)
 Block(2)
 Block(3)
 Block(4)
 Block(5)

julia> collect(BlockRange(mortar([1:2, 3:5])))
2-element Vector{Block{1, Int64}}:
 Block(1)
 Block(2)

This is technically breaking, though I would argue that it is a bug fix because of the inconsistency of the current code design (i.e. I would argue that the previous behavior of BlockRange(::AbstractUnitRange) was incorrect, and therefore was a bug). I'm curious to see if this leads to any downstream failures.

Copy link

codecov bot commented Jun 30, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.82%. Comparing base (0c73bef) to head (5223611).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #472   +/-   ##
=======================================
  Coverage   93.81%   93.82%           
=======================================
  Files          19       19           
  Lines        1681     1683    +2     
=======================================
+ Hits         1577     1579    +2     
  Misses        104      104           

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

@mtfishman
Copy link
Collaborator Author

Looks like downstream tests pass so luckily those packages weren't using the BlockRange constructors that were removed in this PR.

@mtfishman mtfishman merged commit fe6b909 into JuliaArrays:master Jul 1, 2025
16 checks passed
@mtfishman mtfishman deleted the mf/blockrange_struct_constructor branch July 1, 2025 12:59
mtfishman added a commit that referenced this pull request Jul 1, 2025
#472 broken some tests and doctests introduced in #469, I accidentally
merged #469 prematurely without checking that.
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.

Inconsistency in definition of BlockRange(::AbstractUnitRange)
2 participants