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

pretty_table should require_one_based_indexing #110

Closed
yurivish opened this issue Jan 24, 2021 · 7 comments
Closed

pretty_table should require_one_based_indexing #110

yurivish opened this issue Jan 24, 2021 · 7 comments

Comments

@yurivish
Copy link

yurivish commented Jan 24, 2021

Currently pretty_table prints incorrect information about arrays with offset axes.

Julia has a function Base.require_one_based_indexing that can be used to disallow such arrays as input and throw an error instead of exhibiting undefined behavior or returning incorrect results.

julia> using PrettyTables, OffsetArrays

julia> a = OffsetArray(0:10, -5:5)
0:10 with indices -5:5

julia> pretty_table(a)
┌────────┐
│ Col. 1 │
├────────┤
│      6 │
│      7 │
│      8 │
│      9 │
│     10 │
│ #undef │
│ #undef │
│ #undef │
│ #undef │
│ #undef │
│ #undef │
└────────┘
@ronisbr
Copy link
Owner

ronisbr commented Jan 24, 2021

Hi @yurivish,

Thanks for the tip! I will add this to avoid those problems.

However, I just had an amazing idea to support offset arrays. Inside PrettyTables, I can wrap the array inside a custom type (I already do this for Tables.jl). The only functions I need to create for this new type is getindex, size, and isassigned. Hence, I can check inside those if we have an offset array an just do the conversion so that table[1,1] returns the first element and so on.

@yurivish
Copy link
Author

yurivish commented Jan 25, 2021

Interesting idea! Could that new type itself be an OffsetArray that cancels out the offset of the array that was passed to it (which might be an OffsetArray or any other type with offset axes)?

@ronisbr
Copy link
Owner

ronisbr commented Jan 25, 2021

I do not want it to be dependent of OffsetArray. The idea is something like

struct GenericTable
    data
    offset
    size
end

Then we can do something like:

getindex(table::GenericTable, inds...) = table[inds[1] - table.offset[1], inds[2] - table.offset[2]]

This code will not work, but it illustrates my idea :)

@GlenHertz
Copy link
Contributor

Is there a reason too not use firstindex(obj):lastindex(obj) for the indices of the object? I believe those could be defined with @compat for older version of Julia that don't have those functions.

@ronisbr
Copy link
Owner

ronisbr commented May 22, 2021

Is there a reason too not use firstindex(obj):lastindex(obj) for the indices of the object? I believe those could be defined with @compat for older version of Julia that don't have those functions.

Can you please provide more details? lastindex of a Matrix returns the number of elements and I need to know the structure (number of elements in a row and how many rows we have).

@GlenHertz
Copy link
Contributor

I'm trying to do something similar to:

julia> pretty_table(OffsetVector(1:11, -5:5))
┌────────┐
│ Col. 1 │
├────────┤
│      7 │
│      8 │
│      9 │
│     10 │
│     11 │
│ #undef │#undef │#undef │#undef │#undef │#undef │
└────────┘

but I have my own struct that doesn't use 1-based indexing and it also doesn't work (same as above).

I haven't looked into the root issue (it could be from Tables.jl as I implemented the AbstractColumns interface from it to get it compatible with PrettyTables).

@ronisbr ronisbr closed this as completed in ea9823f Sep 3, 2022
@ronisbr
Copy link
Owner

ronisbr commented Sep 3, 2022

Hi @yurivish and @GlenHertz !

First of all, sorry for the HUGE delay. PrettyTables needed a huge rewrite in the algorithm that handles the input table internally before supporting OffsetArrays. Everything is done and we now support OffsetArrays in master! Can you please try?

julia> using PrettyTables

julia> using OffsetArrays

julia> A = OffsetArray(rand(3,3), -2:0, -3:-1);

julia> pretty_table(A, show_row_number = true)
┌─────┬──────────┬──────────┬──────────┐
│ Row │  Col. -3 │  Col. -2 │  Col. -1 │
├─────┼──────────┼──────────┼──────────┤
│  -20.3324280.6981860.251798 │
│  -10.2850710.4342930.987304 │
│   00.5150810.7569210.397783 │
└─────┴──────────┴──────────┴──────────┘

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

3 participants