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

show type for IdOffsetRange #143

Merged
merged 5 commits into from
Sep 13, 2020
Merged

Conversation

jishnub
Copy link
Member

@jishnub jishnub commented Sep 13, 2020

This PR attempts to fix #121 . Now we have

julia> a = OffsetArray([1 2; 3 4], -1:0, 5:6)
2×2 OffsetArray(::Array{Int64,2}, -1:0, 5:6) with eltype Int64 with indices -1:0×5:6:
 1  2
 3  4

julia> axes(a)
(OffsetArrays.IdOffsetRange(-1:0), OffsetArrays.IdOffsetRange(5:6))

@jishnub
Copy link
Member Author

jishnub commented Sep 13, 2020

This PR alleviates this issue in #120 :

Previously

julia> @test 1:3 === OffsetArrays.IdOffsetRange(1:3)
Test Failed at none:1
  Expression: 1:3 === OffsetArrays.IdOffsetRange(1:3)
   Evaluated: 1:3 === 1:3
ERROR: There was an error during testing

Now

julia> @test 1:3 === OffsetArrays.IdOffsetRange(1:3)
Test Failed at REPL[12]:1
  Expression: 1:3 === OffsetArrays.IdOffsetRange(1:3)
   Evaluated: 1:3 === OffsetArrays.IdOffsetRange(1:3)
ERROR: There was an error during testing

@codecov
Copy link

codecov bot commented Sep 13, 2020

Codecov Report

Merging #143 into master will increase coverage by 2.83%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #143      +/-   ##
==========================================
+ Coverage   90.49%   93.33%   +2.83%     
==========================================
  Files           2        2              
  Lines         221      240      +19     
==========================================
+ Hits          200      224      +24     
+ Misses         21       16       -5     
Impacted Files Coverage Δ
src/OffsetArrays.jl 93.87% <100.00%> (+3.48%) ⬆️
src/axes.jl 90.90% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d58dda0...293592e. Read the comment docs.

Copy link
Member

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! I was planning to fix these after finishing my little DemoCards project but you certainly did a great job here!

Could possibly add some show tests to capture the patterns as a regression test. The tools I'd personally use isSuppressor.@capture, or ReferenceTests's @io2str/@test_reference.

You're doing great here, feel free to merge if you think it is ready.

@jishnub jishnub added the needs tests PR needs tests before it can be merged label Sep 13, 2020
jishnub and others added 4 commits September 13, 2020 21:34
* Add constructors for CartesianIndices. A combination of `AbstractUnitRange`s, `Colon` and `CartesianIndices` may be provided to the constructor such that the total number of ranges is equal to the dimension of the parent array.

* Use `convert` instead of accessing indices, leads to the same lowered code but is future-proof
@jishnub jishnub merged commit 1d294f8 into JuliaArrays:master Sep 13, 2020
@johnnychen94
Copy link
Member

FYI you could use git rebase master in showIdOffsetRange branch to keep a clean commit history.

@jishnub
Copy link
Member Author

jishnub commented Sep 14, 2020

I think we need to update the docs to reflect this change

@johnnychen94 johnnychen94 removed the needs tests PR needs tests before it can be merged label Sep 18, 2020
@jishnub jishnub deleted the showIdOffsetRange branch September 21, 2020 05:51
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.

show(::OffsetArrays.IdOffsetRange should show its type
2 participants