Open kahaaga opened 6 months ago
Hm. Not entirely sure about this. Should we instead require the input to be sortable (i.e. the user has to map their data to some sortable format)? That would make maintenance a bit easier.
I think in the future there should be a version implemented for non-sortable collections. But this is extremely low priority because a user can always map their vector to the integers corresponding to unique elements and then use count on the integer. For now we can focus on v3 and hope that some other contributor will add this method in the future.
I think in the future there should be a version implemented for non-sortable collections. But this is extremely low priority because a user can always map their vector to the integers corresponding to unique elements and then use count on the integer.
Agreed. Then I keep this issue open.
There is a difficulty I see in implementing this: there is no julia function, or type trait, that checks for sortability. So it is not clear to me how we would check whether we can call fasthist or not in the first place...
We could check something like
julia> hasmethod(isless, (T, T))
false
with T
the element type of the vector.
We could check something like
julia> hasmethod(isless, (T, T)) false
with
T
the element type of the vector.
Yes, this should work. The fallback would typically just occur when the user has different types of elements in their input vector. Then T
becomes Any
and
julia> T = Any
Any
julia> hasmethod(isless, (T, T))
false
In the current API, the docstring of outcome_space
is
"""
outcome_space(o::OutcomeSpace, x) → Ω
Return a sorted container containing all _possible_ outcomes of `o` for input `x`.
For some estimators the concrete outcome space is known without knowledge of input `x`,
in which case the function dispatches to `outcome_space(o)`.
In general it is recommended to use the 2-argument version irrespectively of estimator.
"""
function outcome_space(o::OutcomeSpace)
For a non-sortable vector, we can't sort the elements. Therefore, we either need to relax this requirement, or disallow using non-sortable data (which I think is too restrictive atm). We could just say that "if your input data isn't sortable, then the outcome space is given in the order of first appearance of the outcomes" or something like that
Yes order of first appearance is fine, but at the moment non sortable data simply domm't work with the pacakge ayways
I haven't done any sort of testing for speed etc, but an obvious (and perhaps the easiest) solution to this issue is precisely what @Datseris says above - convert to the integers (and then convert back again). We already have the infrastructure to do this conversion, in the form of UniqueElementsEncoding
.
is_sortable(x::AbstractVector{T}) where T = hasmethod(isless, (T, T))
fasthist!(x, args...)
is called, instead call and intermediate function fasthist_checksorted!(x, args...)
. It does the following:
a. Checks if the input x
is sortable.
b. If x
is sortable, then use fasthist!
directly.
c. If the input is not sortable, then create a new vector x_ints
that map the unique elements of x
to unique integers. To do so, use UniqueElementsEncoding
on x
and use encode
to map the elements of x
onto the integers.
d. Then pass x_ints
to fasthist!
and obtain the histogram.
e. Use decode
to replace each element of x_ints
with the original input.
f. Optionally optimize counts_and_outcomes(::UniqueElements, x)
by not replacing every element in x_ints
. Because fasthist!
gives a histogram over the unique elements of x_int
, we can get away with only decoding the unique elements in x_ints
.In any case, anyone working on this should wait until #347 is merged.
Currently, we can do
But this only works if
x
is sortable, becausefasthist!
uses sorting internally. We should have a generic (probably much slower) fallback that enables things like this to work too: