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

rmsd function asks for the same type of real number. #935

Open
eoteroe opened this issue Jun 15, 2024 · 3 comments
Open

rmsd function asks for the same type of real number. #935

eoteroe opened this issue Jun 15, 2024 · 3 comments

Comments

@eoteroe
Copy link

eoteroe commented Jun 15, 2024

My target variable was an Int32 and Predictions were Float64. Is there a reason for the function to not handle these two real vectors.

maybe you can allow:

where S<:Number, T<:Number

@ararslan
Copy link
Member

Looking at the functions in src/deviation.jl, where rmsd is defined, it appears none that restrict two input arrays to have the same element type actually need that requirement. We could loosen all of the method signatures to remove the type parameters altogether, which would fix this.

@eoteroe
Copy link
Author

eoteroe commented Jul 9, 2024

Sorry, are you saying it is not correct to change it for code dependencies.

For me this should be working
rmsd([1,2,4],[1.1,2.1,3.9])

thank you.

@ararslan
Copy link
Member

ararslan commented Jul 9, 2024

Ah sorry, my message wasn't clear. I mean that the issue you observed with rmsd is not limited to that function; a lot of the functions in src/deviation.jl are defined like so:

function f(x::AbstractVector{T}, y::AbstractVector{T}) where {T<:Number}
    # Calculations that don't actually require `x` and `y` to have a common element type
end

I think all such cases can be changed to:

function f(x::AbstractVector, y::AbstractVector)
    # ...
end

That is, type parameters for the arrays don't need to be specified at all. Making that change would fix your issue for rmsd, as well as similar issues for a number of other functions.

A bit tangential, and we'd have to check the implementations to make sure this is sound, but we could possibly drop the AbstractVector restrictions on the arguments as well. That would allow using non-Array collections like Tuples. That'd be worth a broader discussion than is relevant for this issue though.

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