Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Generic test for allowing to use isless #319

Closed
bkamins opened this issue Jan 30, 2021 · 4 comments · Fixed by #346
Closed

Generic test for allowing to use isless #319

bkamins opened this issue Jan 30, 2021 · 4 comments · Fixed by #346

Comments

@bkamins
Copy link
Member

bkamins commented Jan 30, 2021

Consider the following example:

julia> x = [1, 2]
2-element Array{Int64,1}:
 1
 2

julia> y = categorical([1])
1-element CategoricalArray{Int64,1,UInt32}:
 1

julia> isless(x[1], y[1])
false

julia> isless(x[2], y[1])
ERROR: KeyError: key 2 not found

This behaviour is very problematic I think. The reason is that given two containers x and y there is no way to check if you can safely use isless to compare their elements. I could run a strict check if eltype of x and y are the same, but it is not desirable very often (e.g. numbers can be compared even if their type is different).

@bkamins
Copy link
Member Author

bkamins commented Feb 1, 2021

@nalimilan - following the discussion on Slack what do you think of requiring a wrapper around non-categorical value when doing < and isless?

I guess == and isequal would stay.

@nalimilan
Copy link
Member

Yes I think that would be OK. That type could also be useful when returning a value from map or ByRow, so that the result is a CategoricalArray.

@tbenst
Copy link

tbenst commented Sep 4, 2021

I would argue this behavior is perfectly fine. That is the definition of a partial order: not every pair of elements can be compared. Forcing users to only use a total order is strictly less expressive.

@bkamins
Copy link
Member Author

bkamins commented Sep 4, 2021

Forcing users to only use a total order is strictly less expressive.

We do not have much room here for discussion. The contract for isless in Julia Base is:

 isless(x, y)

  Test whether x is less than y, according to a fixed total order. isless is not defined on all pairs of values (x, y). However, if it is defined, it is expected to satisfy the following:

    •  If isless(x, y) is defined, then so is isless(y, x) and isequal(x, y), and exactly one of those three yields true.

    •  The relation defined by isless is transitive, i.e., isless(x, y) && isless(y, z) implies isless(x, z).

so it is not allowed to define isless that would not be transitive. This potentially could be changed in Julia 2.0, but I think it is unlikely. Again - as in the other issue - let us discuss, but this is how I understand the contact for isless in Julia Base currently.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants