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

Promote problems #104

Closed
diegozea opened this issue May 5, 2016 · 7 comments
Closed

Promote problems #104

diegozea opened this issue May 5, 2016 · 7 comments

Comments

@diegozea
Copy link

diegozea commented May 5, 2016

I have a type which only accept Float64 values defined as:

@auto_hash_equals immutable Coordinates <: FixedVectorNoTuple{3, Float64}
    x::Float64
    y::Float64
    z::Float64
    function Coordinates(a::NTuple{3, Float64})
        new(a[1], a[2], a[3])
    end
end

I getting this errors trying to divide it with an Int:

image

I have solved it in the meanwhile using Float64(length(...))

@c42f
Copy link
Collaborator

c42f commented May 6, 2016

Yup, this is one of many promotion problems which arise when working with mixed types. I've started work on a rewrite of the map functionality which will hopefully address these type woes. See #105.

@c42f
Copy link
Collaborator

c42f commented May 23, 2016

Ah darn! #105 was going to fix this, but in the process of fixing a whole bunch of other things, your specific type of FSA subtype no longer works, since similar is used in a rather central way in the new implementation, and the default similar doesn't work well with your Coordinates.

We'll need to add something like Coordinates to the test cases, and come up with a coherent story around the role and API of similar for FSAs. In the meantime, if you add the following (caveat, the sz parameter may be a bad API choice for FSAs and may need to change):

Base.similar(::Type{Coordinates}, ::Type{Float64}, sz::Tuple) = sz[1] == 3 ? Coordinates : Vec{sz[1],Float64}
Base.similar{T}(::Type{Coordinates}, ::Type{T}, sz::Tuple) = Vec{sz[1],T}

Then you can do things like:

julia> points = [rand(Coordinates) for _ =1:10]
10-element Array{Coordinates,1}:
 Coordinates(0.7776985119194051,0.29719853453510536,0.10251763435148575)
 Coordinates(0.7379174083324038,0.3296379139698149,0.36391270520448904)
 Coordinates(0.4283095362624538,0.7821447202577865,0.14133737599972518)
 Coordinates(0.7555352303842677,0.520333541723871,0.9614920004560281)
 Coordinates(0.7255490300531384,0.8876056197246123,0.27401014626516274)
 Coordinates(0.0388837350288056,0.9984448841471119,0.6013870295464301)
 Coordinates(0.3917672241105896,0.5867953116084399,0.8013279650490599)
 Coordinates(0.3705099593706276,0.06483885178084137,0.8097820812141754)
 Coordinates(0.9703794802422856,0.934004150420159,0.11295307373487251)
 Coordinates(0.2978305066721063,0.9149579629444147,0.9108417622883043)

julia> mean(points)
Coordinates(0.5494380622376084,0.6315961491112156,0.5079561774109733)

julia> points[1] .< points[2]
Vec(false,true,true)

@diegozea
Copy link
Author

the default similar doesn't work well with your Coordinates.

Is this problem caused by the use of Float64 instead of using a T parameter?

@c42f
Copy link
Collaborator

c42f commented May 23, 2016

Is this problem caused by the use of Float64 instead of using a T parameter

Yes, this is why our tests didn't pick it up (the tests use an RGB{T} type as the prototypical FixedVectorNoTuple), but I don't think you're actually doing anything wrong, I think FixedSizeArrays needs some fixes to make your use case work nicely.

@c42f
Copy link
Collaborator

c42f commented May 26, 2016

We're trying to figure out what to do about this to make your use case smooth - see #116 for API discussion.

@c42f
Copy link
Collaborator

c42f commented Jun 1, 2016

This should now all be sorted out in #118, and you won't even need to overload similar :-)

You get the following:

julia> Coordinates(1,2,3) .< Coordinates(1,2,4)
Vec(false,false,true)

julia> mean([Coordinates(1,1,1) for i=1:10])
Coordinates(1.0,1.0,1.0)

julia> sum([Coordinates(1,1,1) for i=1:10])
Coordinates(10.0,10.0,10.0)

@c42f c42f closed this as completed Jun 1, 2016
@diegozea
Copy link
Author

diegozea commented Jun 1, 2016

Awesome! Thanks!! :D

This issue was closed.
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

2 participants