Skip to content
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

Zero dimensional BlockIndex #431

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

mtfishman
Copy link
Collaborator

@mtfishman mtfishman commented Nov 8, 2024

Fixes #430.

I bumped the minor package version since this is mildly breaking, though I think it would be strange if code was relying on the previous behavior and this PR could be considered a bug fix.

To-do:

  • Add tests.

Copy link

codecov bot commented Nov 8, 2024

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 0.00%. Comparing base (1e5feaa) to head (deddd46).

Files with missing lines Patch % Lines
src/blockindices.jl 0.00% 4 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (1e5feaa) and HEAD (deddd46). Click for more details.

HEAD has 6 uploads less than BASE
Flag BASE (1e5feaa) HEAD (deddd46)
9 3
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #431       +/-   ##
==========================================
- Coverage   93.67%   0.00%   -93.68%     
==========================================
  Files          18      18               
  Lines        1643    1630       -13     
==========================================
- Hits         1539       0     -1539     
- Misses        104    1630     +1526     

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

@mtfishman
Copy link
Collaborator Author

You can see that there were some tests that were explicitly relying on the previous definition that Block()[] == Block(), but the current definition Block()[] == BlockIndex() follows from the property that a[Block()[]] == a[Block()][] so I think that the previous definition must have been a mistake.

@mtfishman
Copy link
Collaborator Author

I see there is a downstream test failure in LazyBandedMatrices.jl here: https://github.com/JuliaLinearAlgebra/LazyBandedMatrices.jl/blob/v0.10.4/test/test_blockconcat.jl#L348 which is calling: https://github.com/JuliaLinearAlgebra/LazyBandedMatrices.jl/blob/v0.10.4/src/blockconcat.jl#L489.

The root cause of that seems to be that Block is not being treated as a scalar in broadcasting, it seems like maybe it was working accidentally before this PR.

@mtfishman
Copy link
Collaborator Author

I see now that Block(4)[] was defined to return Block(4) before because it was matching the behavior of Number, i.e. 4[] === 4, but that seems like a strange choice given that indexing into Block returns BlockIndex (for good reason) in all other cases. I'm testing out changing the broadcasting behavior of Block to define it as a scalar in broadcasting to see if that fixes the LazyBandedMatrices.jl test failure but also allows the BlockArrays.jl tests to pass.

Project.toml Outdated Show resolved Hide resolved
@dlfivefifty
Copy link
Member

dlfivefifty commented Nov 11, 2024

Do we know why the LazyArrays.jl test is failing?

Co-authored-by: Sheehan Olver <[email protected]>
@mtfishman
Copy link
Collaborator Author

Do we know why the LazyArrays.jl test is failing?

I don't know, but it seems to be failing in the same way in other PRs. It looks like an issue with how the LazyArraysBlockArraysExt package extension is configured. I see that BlockArrays is listed as both a dependency and a weak dependency of LazyArrays: https://github.com/JuliaArrays/LazyArrays.jl/blob/v2.2.1/Project.toml#L8, that doesn't seem right.

@dlfivefifty
Copy link
Member

Having it listed as both was necessary and standard to support Julia before extensions so that shouldn’t be the issue…

@mtfishman
Copy link
Collaborator Author

mtfishman commented Nov 12, 2024

Having it listed as both was necessary and standard to support Julia before extensions so that shouldn’t be the issue…

I see. I see a few issues related to package extension loading in Julia 1.11 like JuliaLang/julia#55939 and JuliaLang/julia#56204 but I'm not sure if they are related.

But also I see that LazyArrays.jl requires Julia 1.10 anyway so it seems like they can just be weakdeps now and the code logic https://github.com/JuliaArrays/LazyArrays.jl/blob/v2.2.1/src/LazyArrays.jl#L80-L85 can be removed. EDIT: I'm trying that out in JuliaArrays/LazyArrays.jl#347.

@mtfishman
Copy link
Collaborator Author

I reran the LazyArrays.jl integration test now that JuliaArrays/LazyArrays.jl#347 is merged and registered but there is still an issue.

Maybe the issue is being triggered because BlockBandedMatrix.jl has BlockArrays.jl as a dependency, and the package system is getting confused about that. Again maybe related to JuliaLang/julia#55939.

@jishnub
Copy link
Member

jishnub commented Nov 14, 2024

Many of these issues will be resolved in Julia v1.11.2. Could you check on that branch? Or perhaps we may wait for the release, which should be soon.

@dlfivefifty
Copy link
Member

I thought the lazyarrays extensions were rewritten to avoid this problem…

@mtfishman
Copy link
Collaborator Author

I thought the lazyarrays extensions were rewritten to avoid this problem…

I'm having trouble reproducing the issue locally so it is tough for me to test, JuliaArrays/LazyArrays.jl#347 was my best guess for a quick fix but it must be another issue. As @jishnub said, probably it is best to wait for Julia v1.11.2 to be released and rerun the tests against that version.

@jishnub
Copy link
Member

jishnub commented Nov 15, 2024

You may test against Julia nightly meanwhile, as any feature being backported to v1.11 would be present on nightly

@mtfishman
Copy link
Collaborator Author

Ok, I reran the downstream tests against nightly, and the LazyArrays.jl tests don't hit that same installation issue (one test fails for some unrelated issue, but at least the tests run). So that's a good indication that we can wait for the Julia v1.11.2 release and that will hopefully resolve this issue. I'll revert back to testing against the latest stable release, and I guess we can wait for that release to test again and then merge this.

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.

a[Block()[]] when iszero(ndims(a)) returns a block, not an element
3 participants