Skip to content

Add virtual properties for values and indices#318

Open
mkitti wants to merge 1 commit into
JuliaArrays:masterfrom
mkitti:mkitti/idoffsetrange_getproperty
Open

Add virtual properties for values and indices#318
mkitti wants to merge 1 commit into
JuliaArrays:masterfrom
mkitti:mkitti/idoffsetrange_getproperty

Conversation

@mkitti

@mkitti mkitti commented Dec 25, 2022

Copy link
Copy Markdown
Contributor

This is a full implementation of #316. Virtual properties for values and indices are added and documented.

This differs from the proposed implementation in #316 in that the properties return IdOffsetArray rather than UnitRanges.

Another discovered benefit is the ability to forward the properties to construct another IdOffsetRange.

        _values = 5:8
        _indices = 6:9
        id = IdOffsetRange(values=_values, indices=_indices)
        @test id.values === values(id)
        @test id.values == _values
        @test id.indices === eachindex(id)
        @test id.indices == _indices
        id2 = IdOffsetRange(; id.values, id.indices)
        @test id == id2
        id3 = IdOffsetRange(; id.indices, id.values)
        @test id == id3

@codecov

codecov Bot commented Dec 25, 2022

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 95.16%. Comparing base (08dc371) to head (2945d61).
⚠️ Report is 45 commits behind head on master.

Files with missing lines Patch % Lines
src/axes.jl 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #318      +/-   ##
==========================================
- Coverage   98.67%   95.16%   -3.52%     
==========================================
  Files           5        5              
  Lines         452      434      -18     
==========================================
- Hits          446      413      -33     
- Misses          6       21      +15     

☔ View full report in Codecov by Harness.
📢 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.

@jishnub

jishnub commented Jan 2, 2023

Copy link
Copy Markdown
Member

Looking at the example from #316,

julia> Base.@kwdef struct MyIdOffsetRange
                  values::UnitRange
                  indices::UnitRange
              end
MyIdOffsetRange

julia> x = MyIdOffsetRange(values=5:8, indices=5:8)
MyIdOffsetRange(5:8, 5:8)

julia> x.values
5:8

julia> x.values === x
false

In this example, the returned results correspond exactly to the displayed ones, as both directly read the fields of the struct. However, for an IdOffsetRange, we obtain another IdOffsetRange, which isn't what was displayed.

julia> id
OffsetArrays.IdOffsetRange(values=5:8, indices=6:9)

julia> id.values
OffsetArrays.IdOffsetRange(values=5:8, indices=6:9)

julia> id.values === id
true

This isn't an objection to the PR, but I wonder if we need to be consistent here?

@mkitti

mkitti commented Jan 3, 2023

Copy link
Copy Markdown
Contributor Author

The consistency choice I made here was for

id.values === values(id)

values is part of the API already.

If you really wanted a UnitRange, this could work:

UnitRange(id.values)

For that reason, I added it to the documentation.

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.

2 participants