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

WIP: Try using an advanced indices object that wraps axes #81

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

Conversation

mbauman
Copy link
Member

@mbauman mbauman commented Apr 30, 2017

This supersedes #58. I like this approach better, and the first commit may be worth doing on its own merits:

When asking for the indices of an AxisArray, return a specialized index type
that acts just like normal indices, except that we can also get its
corresponding Axis and we can dispatch on it.

The second commit is where I try to implement reductions (redo #56) in terms of the IndexAxis type. And that doesn't work:

This almost works, but it's not type stable. The trouble is that the
IndexAxis type sometimes wants to know the exact Axis information (like
when used in similar), but other times it only cares about the Axis name
(like when choosing the dimensions for a reduction). Specifically, this branch
runs into trouble with this base method:

    function reduced_indices(inds::Indices{N}, d::Int, rd::AbstractUnitRange) where N
        d < 1 && throw(ArgumentError("dimension must be ≥ 1, got $d"))
        if d == 1
            return (oftype(inds[1], rd), tail(inds)...)
        elseif 1 < d <= N
            return tuple(inds[1:d-1]..., oftype(inds[d], rd), inds[d+1:N]...)::typeof(inds)

I've broken the contract that oftype—that is, convert—returns an object
of exactly the requested type. But here I simply want a new IndexAxis object
that has the same name and wraps the given rd range.

I need to call it quits on this for now, but it's definitely better than #58. Any interest in pursuing this further? I'd be curious if this allows more base algorithms to get axis smarts for free, even if we don't use it for reductions.

When asking for the indices of an AxisArray, return a specialized index type
that acts just like normal indices, except that we can also get its
corresponding Axis and we can dispatch on it.
This _almost_ works, but it's not type stable. The trouble is that the
`IndexAxis` type _sometimes_ wants to know the exact Axis information (like
when used in `similar`), but other times it only cares about the Axis name
(like when choosing the dimensions for a reduction).  Specifically, this branch
runs into trouble with this base method:

```
    function reduced_indices(inds::Indices{N}, d::Int, rd::AbstractUnitRange) where N
        d < 1 && throw(ArgumentError("dimension must be ≥ 1, got $d"))
        if d == 1
            return (oftype(inds[1], rd), tail(inds)...)
        elseif 1 < d <= N
            return tuple(inds[1:d-1]..., oftype(inds[d], rd), inds[d+1:N]...)::typeof(inds)
```

I've broken the contract that `oftype`—that is, `convert`—returns an object
of exactly the requested type.  But here I simply want a new IndexAxis object
that has the same name and wraps the given `rd` range.
@mbauman mbauman changed the base branch from master to mb/constructor2 April 30, 2017 22:20
@mbauman
Copy link
Member Author

mbauman commented Apr 30, 2017

Tests pass if I make convert(::Type{IndexAxis}, …) follow the rules, but then this fails (since it tries to convert the new OneTo(1) dimension to a unitful quantity:

julia> A = AxisArray(collect(reshape(1:15,3,5)), 1s:1s:3s, 1:5)
2-dimensional AxisArray{Int64,2,...} with axes:
    :row, 1 s:1 s:3 s
    :col, 1:5
And data, a 3×5 Array{Int64,2}:
 1  4  7  10  13
 2  5  8  11  14
 3  6  9  12  15

julia> sum(A, 1)
ERROR: DimensionError: s and 1 are not dimensionally compatible.

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.

1 participant