Generalize the element type of `BlockedUnitRange` #337

mtfishman · 2024-03-19T22:46:05Z

Fixes #336.

This is meant to be a minimal implementation that gets tests passing, there may be a few different design choices to make for certain functions. I also need to check that the element type is actually preserved appropriately, such as in functions like blockfirsts, blocklasts, etc.

I'm not sure what we want for eltype(blocklengths(r)) (i.e. should it equal eltype(r) or Int?), since length(::AbstractUnitRange) doesn't match eltype(::AbstractUnitRange):

julia> typeof(length(0x01:0x03))
Int64

codecov · 2024-03-19T22:51:34Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.47%. Comparing base (b361279) to head (287c1aa).

Additional details and impacted files

@@               Coverage Diff               @@
##           release-1.0     #337      +/-   ##
===============================================
+ Coverage        95.42%   95.47%   +0.05%     
===============================================
  Files               17       17              
  Lines             1530     1548      +18     
===============================================
+ Hits              1460     1478      +18     
  Misses              70       70

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

mtfishman · 2024-03-20T00:36:26Z

Looks like there are some downstream test failures caused by adding an extra type parameter to BlockedUnitRange.

jishnub · 2024-03-20T07:11:42Z

Probably a non-breaking implementation needs to be in the non-standard form BlockedUnitRange{R,T}, where T is obtained from R in the inner constructor.

dlfivefifty · 2024-03-20T09:23:44Z

I’m thinking we tag v1 after this is done so it’s fine if it’s breaking

mtfishman · 2024-03-20T12:04:27Z

I’m thinking we tag v1 after this is done so it’s fine if it’s breaking

I'm guessing that most use cases of BlockedUnitRange can be replaced with BlockedOneTo, for example axes of most AbstractBlockArray subtypes should be BlockedOneTo, and also blockedrange should output BlockedOneTo (see related discussion in #334). Perhaps it is better to wait for a v1 release after those changes are made?

dlfivefifty · 2024-03-20T20:07:08Z

Yes sorry I meant tag v1 once we also add BlockedOneTo

mtfishman · 2024-03-21T18:32:35Z

I think this is ready for review. Some discussion points are:

What should the element types of blocklengths and length be? Currently they are the eltype of the BlockedUnitRange, but that could be changed. It seems to be conventional for length to be Int in Julia, but that choice probably doesn't make sense for ranges, since they can be BigInt or even infinite length. There are tests of the types of the outputs of the various block axes interface functions so you can see the current behavior.
I've refactored the constructors of BlockedUnitRange to make sure that the fields r.first and r.lasts have types consistent with the element type (and each other). My strategy was to promote r.lasts to a common type if it is a tuple (i.e. it is converted to an NTuple), and also convert r.first to eltype(r.lasts) if needed.
How should this be coordinated with the developed of a BlockedOneTo type (and also possibly a AbstractBlockedUnitRange supertype)? Ideally that would use this as a starting point, but merging this into the master branch commits the master branch to making a breaking change, which likely should be put off until a v1 release. Could this get merged into a new v1 branch where further v1 developments like BlockedOneTo can occur?

jishnub · 2024-03-21T18:45:03Z

length for BigInt ranges returns a BigInt, so I don't think we need to be constrained to Int.
Yes, we should create a branch for v1 and change the base branch for this PR to point to that. I'm also planning a BlockedOneTo PR soon (it's almost ready at this point).

jishnub · 2024-03-22T03:40:11Z

There already appears to be a release-1.0 branch from a year back. Could you change the target branch of the PR to that one?

jishnub · 2024-04-07T18:37:33Z

Gentle bump, could you rebase this on the updated release-1.0 branch?

mtfishman · 2024-04-08T20:28:29Z

Will do.

mtfishman · 2024-04-14T02:05:53Z

@jishnub I merged the latest release-1.0 into this branch.

I assume we will want to generalize the element type of BlockedOneTo in a similar way but it seems like that could be done in a follow-up PR.

In the latest commit I made the type constraints of the inner _BlockedUnitRange constructors stricter, so that first and lasts are expected to have more uniform types, and outer constructors use type promotion to make the types more uniform, for example now there is this behavior:

julia> BlockArrays._BlockedUnitRange(UInt8(1), [1, 2, 3])
3-blocked 3-element BlockedUnitRange{Int64, Vector{Int64}}:
 1
 ─
 2
 ─
 3

julia> BlockArrays._BlockedUnitRange(UInt8(1), (UInt8(1), 2, 3))
3-blocked 3-element BlockedUnitRange{Int64, Tuple{Int64, Int64, Int64}}:
 1
 ─
 2
 ─
 3

whereas before the types of the fields first and lasts where not uniform in those cases. Let me know if you think that is the right behavior, and the right way to design that code logic. The way I went about it involved constraining the type of the lasts field to either be a Tuple or AbstractVector, which seems reasonable but I'm not sure if you have other cases in mind for what the field might be.

jishnub · 2024-04-21T11:49:54Z

I think this looks good at this point. There are some issues like last(blockedrange(2, UInt(1):∞)) throwing errors, but these may not be related to this PR and may be addressed separately. At this point, we just need tests for the flagged lines for which coverage is missing.

mtfishman · 2024-04-22T01:02:59Z

I think this looks good at this point. There are some issues like last(blockedrange(2, UInt(1):∞)) throwing errors, but these may not be related to this PR and may be addressed separately. At this point, we just need tests for the flagged lines for which coverage is missing.

Sounds good, I'll add some more tests to improve the coverage.

mtfishman · 2024-04-22T14:52:42Z

Ok I think everything is covered now.

jishnub · 2024-04-23T16:00:34Z

Great, let's merge this and then figure out the other issues

* Compact show for `BlockRange` (#248) * Compact show for BlockRange * update docstrings * don't specialize show for zero dim * fix missing io in print * missing show tests * show for BlockIndexRange * Bump version to v1.0.0-dev * Add `BlockedOneTo` as the axis type for a `BlockedArray` (#348) * Add BlockedOneTo * axes for AbstractBlockedUnitRange returns BlockedOneTo * Rewrite test using blockedrange instead of BlockedUnitRange * Update BlockedUnitRange docstring and add for BlockedOneTo/blockedrange * Show for BlockedOneTo * Blocklengths for OrdinalRange block sizes * Update docs * Return BlockedOneTo in indexing with BlockRange * Be less fussy in show tests * Require 1-based lasts in blockedrange * Disallow offset arrays in BlockedUnitRange * undo unnecessary doc change * Test conversions between BlockedOneTo and BlockedUnitRange * Reduce the number of convert methods * Remove axes1 specialization * Disallow offset block axes and blocks in BlockArray constructor * Remove unused axes method * Infinite broadcast tests (#383) * Specialize blockedrange BroadcastStyle for LazyArrayStyle (#384) * Specialize blockedrange BroadcastStyle for LazyArrayStyle * Add compat for LazyArrays * Define dataids for PseudoBlockArrays (#364) (#385) * Define dataids for PseudoBlockArrays (#364) * Don't use dataids of axes * Banded Matrix extension (#388) * Move BandedMatrices+BlockArrays code in BlockBandedMatrices to extension * Bump julia-actions/setup-julia from 1 to 2 (#387) Bumps [julia-actions/setup-julia](https://github.com/julia-actions/setup-julia) from 1 to 2. - [Release notes](https://github.com/julia-actions/setup-julia/releases) - [Commits](julia-actions/setup-julia@v1...v2) --- updated-dependencies: - dependency-name: julia-actions/setup-julia dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Move over blockbanded code * Add tests * Update Project.toml * add tests * Update Project.toml --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Generalize the element type of `BlockedUnitRange` (#337) * Allow more general BlockUnitRange element types * Restrict element type * Get tests passing * Fix some tests * Fix some doctests * Skip broken test in Julia v1.6 * Better support for unitful numbers * Fix tests * Stricter types in _BlockedUnitRange * Improve tests coverage * Allow non-Int eltypes in BlockedOneTo (#395) * Allow non-Int eltypes in BlockedOneTo * Specific constructors in BlockedOneTo * Restrict eltype to integers in BlockedOneTo constructors * Tests for construction from a Tuple * Move LazyArrays extension to LazyArrays.jl (#393) * Move LazyArrays extension to LazyArrays.jl * remove LazyArrays * Move over OneToCumsum This was type piracy in LazyBandedMatrices.jl * Update blockaxis.jl * move out InfiniteArrays.jl tests * Use FillArrays accumulate overloads (#397) * Bump julia-actions/setup-julia from 1 to 2 (#387) Bumps [julia-actions/setup-julia](https://github.com/julia-actions/setup-julia) from 1 to 2. - [Release notes](https://github.com/julia-actions/setup-julia/releases) - [Commits](julia-actions/setup-julia@v1...v2) --- updated-dependencies: - dependency-name: julia-actions/setup-julia dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Map imported names to correct parentmodules (#391) * Remove unused imported names (#392) * Don't import Base.Cartesian (#394) I don't think this is being used anymore * Use FillArrays accumulate * Bump julia-actions/cache from 1 to 2 (#398) Bumps [julia-actions/cache](https://github.com/julia-actions/cache) from 1 to 2. - [Release notes](https://github.com/julia-actions/cache/releases) - [Commits](julia-actions/cache@v1...v2) --- updated-dependencies: - dependency-name: julia-actions/cache dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Update Project.toml * Update Project.toml * try running Pkg.update() --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Jishnu Bhattacharya <[email protected]> * Redefine blocksizes (#399) * Redefine blocksizes * Revert change to docstring * Add tests, fix some tests, add docstring * Fix more tests * Add test Project.toml * Git ignore vim temp files * Fixes to test Project.toml * Another test Project.toml fix * Move code, change type design, better code coverage * Backwards compatibility. Fix doctest. * Fix tests * Redesign BlockSizes to be AbstractArray subtype * Rename PseudoBlockArray to BlockedArray (#401) * v1.0, add README * rename files * Update README.md --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Jishnu Bhattacharya <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Matt Fishman <[email protected]>

mtfishman added 3 commits March 19, 2024 18:28

Allow more general BlockUnitRange element types

1c8024e

Restrict element type

789b625

Get tests passing

7700c05

mtfishman and others added 5 commits March 21, 2024 11:53

Fix some tests

c685cf6

Merge branch 'master' into generalize_blockedunitrange

e69d525

Fix some doctests

ed2db6c

Skip broken test in Julia v1.6

099eafc

Better support for unitful numbers

d7d8640

mtfishman changed the title ~~[WIP] Generalize the element type of BlockedUnitRange~~ Generalize the element type of BlockedUnitRange Mar 21, 2024

mtfishman changed the base branch from master to release-1.0 March 22, 2024 03:55

mtfishman and others added 3 commits April 13, 2024 20:41

Merge branch 'release-1.0' into generalize_blockedunitrange

02fcfc9

Fix tests

d6537ce

Stricter types in _BlockedUnitRange

43bdb7f

Improve tests coverage

287c1aa

jishnub merged commit 8639776 into JuliaArrays:release-1.0 Apr 23, 2024
14 checks passed

mtfishman deleted the generalize_blockedunitrange branch April 23, 2024 16:29

This was referenced Jun 7, 2024

Generalize BlockedUnitRange to element types besides Int #335

Closed

Generalize BlockArray/BlockedArray to non-integer block lengths #405

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generalize the element type of `BlockedUnitRange` #337

Generalize the element type of `BlockedUnitRange` #337

mtfishman commented Mar 19, 2024

codecov bot commented Mar 19, 2024 •

edited

Loading

mtfishman commented Mar 20, 2024

jishnub commented Mar 20, 2024

dlfivefifty commented Mar 20, 2024

mtfishman commented Mar 20, 2024 •

edited

Loading

dlfivefifty commented Mar 20, 2024

mtfishman commented Mar 21, 2024 •

edited

Loading

jishnub commented Mar 21, 2024

jishnub commented Mar 22, 2024

jishnub commented Apr 7, 2024

mtfishman commented Apr 8, 2024

mtfishman commented Apr 14, 2024 •

edited

Loading

jishnub commented Apr 21, 2024 •

edited

Loading

mtfishman commented Apr 22, 2024

mtfishman commented Apr 22, 2024

jishnub commented Apr 23, 2024

Generalize the element type of BlockedUnitRange #337

Generalize the element type of BlockedUnitRange #337

Conversation

mtfishman commented Mar 19, 2024

codecov bot commented Mar 19, 2024 • edited Loading

Codecov Report

mtfishman commented Mar 20, 2024

jishnub commented Mar 20, 2024

dlfivefifty commented Mar 20, 2024

mtfishman commented Mar 20, 2024 • edited Loading

dlfivefifty commented Mar 20, 2024

mtfishman commented Mar 21, 2024 • edited Loading

jishnub commented Mar 21, 2024

jishnub commented Mar 22, 2024

jishnub commented Apr 7, 2024

mtfishman commented Apr 8, 2024

mtfishman commented Apr 14, 2024 • edited Loading

jishnub commented Apr 21, 2024 • edited Loading

mtfishman commented Apr 22, 2024

mtfishman commented Apr 22, 2024

jishnub commented Apr 23, 2024

Generalize the element type of `BlockedUnitRange` #337

Generalize the element type of `BlockedUnitRange` #337

codecov bot commented Mar 19, 2024 •

edited

Loading

mtfishman commented Mar 20, 2024 •

edited

Loading

mtfishman commented Mar 21, 2024 •

edited

Loading

mtfishman commented Apr 14, 2024 •

edited

Loading

jishnub commented Apr 21, 2024 •

edited

Loading