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

properly support offset indices #992

Merged
merged 4 commits into from
Oct 16, 2023

Conversation

simeonschaub
Copy link
Contributor

The current behavior for offset indices doesn't seem to be actually useful and is quite inconsistent. If declared as @variables k[0:3], k at first glance seems to just be 1:4, i.e. k[0] throws a BoundsError and k[4] is valid, but axes can in some cases tell a different story:

julia> axes(Symbolics.unwrap(k))
(1:4,)

julia> axes(Symbolics.unwrap(k), 1)
0:3

This PR fixes these cases by properly respecting the offsets, so k is now indexed 0 through 3 as expected. There might still be buggy behavior in other places that don't properly handle offset indices, but this at least fixes the basic cases. Unfortunately I don't know if people are relying on the current buggy behavior, so this might need to be taken into consideration

@codecov-commenter
Copy link

codecov-commenter commented Oct 8, 2023

Codecov Report

Merging #992 (b81e67d) into master (e2f5726) will increase coverage by 0.16%.
Report is 4 commits behind head on master.
The diff coverage is 54.54%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master     #992      +/-   ##
==========================================
+ Coverage   77.87%   78.03%   +0.16%     
==========================================
  Files          27       27              
  Lines        3371     3378       +7     
==========================================
+ Hits         2625     2636      +11     
+ Misses        746      742       -4     
Files Coverage Δ
src/array-lib.jl 78.12% <100.00%> (+1.04%) ⬆️
src/arrays.jl 74.81% <44.44%> (-0.42%) ⬇️

... and 5 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@simeonschaub
Copy link
Contributor Author

simeonschaub commented Oct 8, 2023

Ok, there's one reference test failure on 1.6 remaining. Looks like the ordering of two of the multiplies is flipped on 1.6, would it be ok to only run that test on Julia stable? The other failures all look unrelated.

Ah, nvm! I missed 2993aa6, should be unrelated as well

@simeonschaub

This comment was marked as outdated.

@simeonschaub
Copy link
Contributor Author

Ok, once tests pass this should be good to go

simeonschaub and others added 4 commits October 13, 2023 16:32
@ChrisRackauckas ChrisRackauckas merged commit e4b06dc into JuliaSymbolics:master Oct 16, 2023
12 of 17 checks passed
@simeonschaub simeonschaub deleted the offset-idx branch October 16, 2023 22:02
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.

3 participants