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

[Feature Request] Add values and indices virtual properties to IdOffsetRange #316

Open
mkitti opened this issue Dec 24, 2022 · 4 comments
Open

Comments

@mkitti
Copy link
Contributor

mkitti commented Dec 24, 2022

Currently when you show an IdOffsetRange it displays indices and values:

julia> a = OffsetArrays.IdOffsetRange(1:4, 4)
OffsetArrays.IdOffsetRange(values=5:8, indices=5:8)

The intuition from this display is that an IdOffsetRange might have a values and indices property. However, this is not the case.

julia> a.values
ERROR: type IdOffsetRange has no field values
Stacktrace:
 [1] getproperty(x::OffsetArrays.IdOffsetRange{Int64, UnitRange{Int64}}, f::Symbol)
   @ Base ./Base.jl:38
 [2] top-level scope
   @ REPL[112]:1

julia> a.indices
ERROR: type IdOffsetRange has no field indices
Stacktrace:
 [1] getproperty(x::OffsetArrays.IdOffsetRange{Int64, UnitRange{Int64}}, f::Symbol)
   @ Base ./Base.jl:38
 [2] top-level scope
   @ REPL[113]:1

I suggest that we add those properties based on what is shown via show:

Base.show(io::IO, r::IdOffsetRange) = print(io, IdOffsetRange, "(values=",first(r), ':', last(r),", indices=",first(eachindex(r)),':',last(eachindex(r)), ")")

An example implementation is as follows.

julia> Base.getproperty(r::IdOffsetRange, s::Symbol) = hasfield(typeof(r),s) ? getfield(r,s) : 
                                                       s == :values ? (first(r):last(r)) :
                                                       s == :indices ? (first(eachindex(r)) : last(eachindex(r))) :
                                                       error("type IdOffsetRange has no property $s")

julia> Base.propertynames(r::IdOffsetRange) = (fieldnames(typeof(r))..., :values, :indices)

julia> a.offset
4

julia> a.parent
1:4

julia> a.indices
5:8

julia> a.values
5:8
@mkitti
Copy link
Contributor Author

mkitti commented Dec 24, 2022

One issue is the difference between Base.values and .values here:

julia> values(a)
IdOffsetRange(values=5:8, indices=5:8)

julia> a.values
5:8

@jishnub
Copy link
Member

jishnub commented Dec 25, 2022

As i understand, the displayed form is meant to act as a constructor, and not to indicate properties. Are there other array types where property access corresponds to the displayed form?

@mkitti
Copy link
Contributor Author

mkitti commented Dec 25, 2022

The default display for a struct type is to present a constructor syntax given it's fields.

julia> struct Foo
           x::Int
           y::Int
       end

julia> Foo(3,2)
Foo(3, 2)

The constructor itself appears like it was generated by Base.@kwdef where the keywords correspond to fields.

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

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

If the default display were as follows, I would understand more clearly that the fields correspond to the displayed arguments.

julia> Base.show(io::IO, r::IdOffsetRange) = print(io, IdOffsetRange, "(", r.parent, ", ", r.offset, ")")

julia> a = OffsetArrays.IdOffsetRange(1:4, 4)
IdOffsetRange(1:4, 4)

However, a deliberate choice was made to display the keyword form. It's frustrating that one cannot extract the values and indices shown directly based on that presentation.

@mkitti mkitti changed the title Add values and indicies virtual properties to IdOffsetRange Add values and indices virtual properties to IdOffsetRange Dec 25, 2022
@mkitti mkitti changed the title Add values and indices virtual properties to IdOffsetRange [Feature Request] Add values and indices virtual properties to IdOffsetRange Dec 25, 2022
@mkitti
Copy link
Contributor Author

mkitti commented Dec 26, 2022

Happy holidays!

Thinking about this further, I think some additional documentation on how to extract the values and indices would go a long way. To that end, I've submitted #317 .

In comparison, #318 seems to be quite involved to fix, although I still plan to do so.

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

No branches or pull requests

2 participants