JuliaGeometry / GeometryTypes.jl

Geometry types for Julia
Other
67 stars 41 forks source link

Inconsistency between minimum (maximum) and extrema #174

Closed CarpeNecopinum closed 5 years ago

CarpeNecopinum commented 5 years ago

Base.extrema works as expected (or at least gives some reasonable answer) for arrays of GeometryTypes.Vec, but Base.minimum (maximum) does not...

extrema([Vec2f0(-1,1), Vec2f0(-0.5, 0.5)])[1]
2-element Vec{2,Float32} with indices SOneTo(2):
 -1.0
  0.5

minimum([Vec2f0(-1,1), Vec2f0(-0.5, 0.5)])
2-element Vec{2,Float32} with indices SOneTo(2):
 -1.0
 -1.0

Which is pretty jarring, as the -1.0 isn't even a valid option for the second component really.

reducing with Base.min produces yet another, but still at least to some degree understandable result

reduce(min, [Vec2f0(-1,1), Vec2f0(-0.5, 0.5)])
2-element Vec{2,Float32} with indices SOneTo(2):
 -1.0
  1.0
CarpeNecopinum commented 5 years ago

This can be fixed by replacing

function Base.minimum(a::AbstractVector{T}) where T <: FixedVector
    reduce((x, v)-> min.(x[1], v), a; init=T(typemax(eltype(T))))
end
function Base.maximum(a::AbstractVector{T}) where T <: FixedVector
    reduce((x, v)-> max.(x[1], v), a; init=T(typemin(eltype(T))))
end

(at https://github.com/JuliaGeometry/GeometryTypes.jl/blob/master/src/FixedSizeArrays.jl#L172)

with

function Base.minimum(a::AbstractVector{T}) where T <: FixedVector
    reduce((x, v)-> min.(x, v), a; init=T(typemax(eltype(T))))
end
function Base.maximum(a::AbstractVector{T}) where T <: FixedVector
    reduce((x, v)-> max.(x, v), a; init=T(typemin(eltype(T))))
end

If desired, I'll set up a PR for this.

SimonDanisch commented 5 years ago

Oh wow, that seems like a pretty basic bug :-O A PR would be lovely :)

JeffBezanson commented 5 years ago

That's not what maximum is supposed to do. It's supposed to return the largest element from the array according to isless.

SimonDanisch commented 5 years ago

True, I guess... Is there a function that calculates the extrema per dimension, like what we do here?

CarpeNecopinum commented 5 years ago

A bit of a stream of consciousness regarding that topic:

With a collection of points represented as a regular julia matrix, we can at least get the corners of the bounding box by using maximum / minimum with the dims kwarg. With a Vector of GeometryTypes points/vectors, we don't really have that option.

(Admittedly, I find the definition of isless on Arrays kind of weird in Julia.)

From my point of view over other geometry-related libraries, it appears a kind of consensus, that the min of two vectors/points should be their component-wise minimum, not the smaller one in lexicographic ordering. With that in mind, I find leaving in maximum and minimum functions based on lexicographic comparison kind of dangerous.

From a purely mathematical view, I'd consider it the cleanest to make isless not work for points/vectors, so maximum would break down as well. Then we could add a bb_min / bb_max to get the corners of the bounding box. Though that would also mean that they couldn't <: AbstractArray anymore, so not really an option.