Skip to content

Add isordered #26

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.

Sign up for GitHub

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

Open
bkamins wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Add isordered #26

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
105972e
Add isordered
bkamins Oct 21, 2020
63b920b
Update Project.toml
bkamins Oct 21, 2020
22776a0
Update runtests.jl
bkamins Oct 21, 2020
c32bc8b
Update Project.toml
bkamins Oct 22, 2020
6ca10ba
Apply suggestions from code review
bkamins Oct 22, 2020
de66603
Update src/DataAPI.jl
bkamins Dec 14, 2020
87c664c
Merge branch 'master' into bkamins-patch-1
bkamins Dec 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion Project.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
name = "DataAPI"
uuid = "9a962f9c-6df0-11e9-0e5d-c546b8b5ee8a"
authors = ["quinnj <[email protected]>"]
version = "1.3.0"
version = "1.4.0"

[deps]
Dates = "ade2ca70-3891-5945-98fb-dc099432e06a"

[compat]
julia = "1"
Expand Down
25 changes: 25 additions & 0 deletions src/DataAPI.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
module DataAPI

import Dates

"""
defaultarray(T, N)

Expand Down Expand Up @@ -89,6 +91,9 @@ If the collection is not sortable then the order of levels is unspecified.

Contrary to [`unique`](@ref), this function may return values which do not
actually occur in the data, and does not preserve their order of appearance in `x`.

If a type of `A` implements `levels` with a meaningful order then it should also
implement `isordered` as by defualt `isordered` returns `false`.
Copy link
Member

@nalimilan nalimilan Oct 22, 2020

Choose a reason for hiding this comment

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

Suggested change
implement `isordered` as by defualt `isordered` returns `false`.
implement `isordered` as by default `isordered` returns `false`.

Copy link
Member

@nalimilan nalimilan Oct 22, 2020

Choose a reason for hiding this comment

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

This sentence raises many issues about what we really want isordered to mean. Types that implement levels are AbstractArrays, and in general they don't know with what eltypes they will be used. So the ordering doesn't depend on them.

CategoricalArray is a very special case, but even there I would say that isordered works because the element type is CategoricalValue, which supports <. So everything is defined by the element type.

Likewise, below, I wouldn't say that < is consistent with levels. Since < is defined on elements, not on arrays, and levels is defined on arrays, there's only one way to make them consistent: have levels return values sorted according to </isless, which is what the levels fallback does. So actually it's < and isless that have to be consistent (this is probably always the case).

What could happen is that a special array type could define levels to return something which isn't consistent with </isless. But that doesn't mean that entries in the arrays wouldn't be comparable using <. Do we want to support this? That sounds a bit weird, but it could happen e.g. with a special type similar to CategoricalArray but which wouldn't wrap values in CategoricalValue (IOW a type similar to PooledArray but for which levels would return the pool). Should this kind of object define isordered to return false? That would be weird since entries could still be compared using < -- but the result would be different from the order in levels...

Copy link
Member Author

@bkamins bkamins Oct 22, 2020

Choose a reason for hiding this comment

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

This is a good point. I was thinking how to put it best and I just "kind of" reused what CategoricalArrays.jl defined:

Test whether entries in A can be compared using <, > and similar operators, using the ordering of levels.

Which implies that isordered should tell us if what levels(A) returns is ordered if we use < for comparison of these elements (so technically if issorted(levels(A)) returns true).

In particular I understood that if levels returns something for a type that supports < that fails issorted(levels(A)) then isordered should return false.

This was my understanding of the original intent of what isordered should mean i.e. that two conditions must be met:

  1. eltype of A supports < etc.
  2. levels returns values in order consistent with it

(but maybe I was wrong).

But if I was right then levels and isordered must be in sync. This is different from Base.Ordered(), as Base.Ordered() is unrelated with levels.

Copy link
Member

@nalimilan nalimilan Oct 25, 2020

Choose a reason for hiding this comment

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

Yeah, the problem is that isordered was initially designed with only CategoricalArrays in mind, and for them it happens to be the case that isordered, levels and </isless are all consistent. So now to extend it we have to decide whether that should always be the case (and document it) or not. That would be the simplest and most obvious definition. But it's also quite strict as e.g. a custom array for which levels(x) == ["c", "b", "a"] wouldn't respect it. In short, I think we should know what we want to use this trait for to choose the most useful solution.

@quinnj You mentioned at #23 (comment) that Arrow.jl could use this for a custom dict-encoded type. How would that type behave? How would it implement isordered?

Copy link
Member

@quinnj quinnj Oct 26, 2020

Choose a reason for hiding this comment

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

The arrow spec says:

/// By default, dictionaries are not ordered, or the order does not have
/// semantic meaning. In some statistical, applications, dictionary-encoding
/// is used to represent ordered categorical data, and we provide a way to
/// preserve that metadata here
isOrdered: bool;

So it's not really doing much by itself other than preserving the metadata. The DictEncoded array type in Arrow.jl is basically just a PooledArray, but also includes the boolean isOrdered field; but any further "semantics" would need to be handled by the user or custom comparison code (or in the example of converting to a CategoricalArray, it could query the DataAPI.isordered property).

Copy link
Member Author

@bkamins bkamins Oct 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I would be OK with both. The original definition came from CategoricalArrays.jl which are special, so we can be more flexible here I think (in which case isordered is giving us roughly the same as Ordered trait in Julia Base).

EDIT: so then the question is if we need both (the benefit of having it in DataAPI.jl is that we can change things more easily than in Julia Base).

Copy link
Member

@nalimilan nalimilan Oct 27, 2020

Choose a reason for hiding this comment

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

If Arrow.DictEncoded doesn't want to expose the custom order of levels using generic API, then I don't think it makes sense for it to implement isordered. Otherwise that function will be useless as it won't allow writing generic code that can do something with levels: you would know there's an order, but you wouldn't be able to retrieve it.

One issue with Base.OrderStyle is that it's supposed to work when passing only the type, but for CategoricalArray it's a runtime property...

Copy link
Member

@quinnj quinnj Oct 27, 2020

Choose a reason for hiding this comment

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

But Arrow.DictEncoded does want to expose the custom order of levels? I'd like to implement DataAPI.refarray, DataAPI.refpool, DataAPI.levels and DataAPI.isordered on DictEncoded, so it can participate in all the optimizations built around those.

Copy link
Member

@nalimilan nalimilan Oct 27, 2020

Choose a reason for hiding this comment

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

OK, I thought you didn't want that as you said:

Personally, I don't think we should require eltype of A supports < etc. and levels returns values in order consistent with it, if we don't have to.

Then we can require isordered to be consistent with levels at least, but mention that it may be inconsistent with < and isless. But we shouldn't say that isordered should return true for types that implement levels, as it wouldn't always be the case for CategoricalArray and Arrow.DictEncoded.

Copy link
Member

@quinnj quinnj Oct 29, 2020

Choose a reason for hiding this comment

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

Ok, yeah, that makes sense to me. Yeah, for Arrow.DictEncoded, isordered is an independent property.

"""
function levels(x)
T = Base.nonmissingtype(eltype(x))
Expand All @@ -100,6 +105,26 @@ function levels(x)
levs
end

"""
isordered(A)

Test whether entries in `A` can be compared using `<`, `>` and similar operators,
using the ordering of `levels`.

If a type of `A` implements `levels` with a meaningful order then it should also
Copy link
Member Author

@bkamins bkamins Dec 28, 2020

Choose a reason for hiding this comment

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

@nalimilan - could you please comment in the implementation how your proposal should be incorporated? Thank you!

implement `isordered` as by defualt `isordered` returns `false`.
Copy link
Contributor

@Nosferican Nosferican Dec 14, 2020

Choose a reason for hiding this comment

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

Fix typo, default

"""
function isordered end

isordered(::AbstractArray{<:Union{Missing, AbstractString}}) = true
isordered(::AbstractArray{<:Union{Missing, Real}}) = true
isordered(::AbstractArray{<:Union{Missing, Symbol}}) = true
isordered(::AbstractArray{<:Union{Missing, AbstractChar}}) = true
isordered(::AbstractArray{<:Union{Missing, Dates.Period}}) = true
isordered(::AbstractArray{<:Union{Missing, Dates.TimeType}}) = true
isordered(::AbstractArray{Missing}) = true
isordered(::AbstractArray{<:Any}) = false

"""
Between(first, last)

Expand Down
12 changes: 12 additions & 0 deletions test/runtests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,5 +84,17 @@ end
end

end

@testset "isordered" begin

@test isordered([1])
Copy link
Member

@quinnj quinnj Oct 22, 2020

Choose a reason for hiding this comment

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

Tests are failing because isordered isn't exported from DataAPI; I'd rather not export to be consistent with other DataAPI definitions, but I also think it's probably a fairly safe case if we really want to.

Copy link
Member Author

@bkamins bkamins Oct 22, 2020

Choose a reason for hiding this comment

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

Agreed. I will fix it - every time I think that the PR is simple (and I just edit it in the web browser) I mess up things. I should be more careful.

@test isordered(Union{Real,Missing}[1])
@test isordered(["1"])
@test isordered(['1'])
@test !isordered(Any[1])
@test !isordered(["1", '1'])
@test !isordered(Union{Char, String}["1", '1'])

end

end # @testset "DataAPI"