-
Notifications
You must be signed in to change notification settings - Fork 193
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
Allow only 1-based indexed vectors in AbstractWeights #791
Conversation
""" | ||
macro weights(name) | ||
return quote | ||
mutable struct $name{S<:Real, T<:Real, V<:AbstractVector{T}} <: AbstractWeights{S, T, V} | ||
values::V | ||
sum::S | ||
end | ||
$(esc(name))(values::AbstractVector{<:Real}) = $(esc(name))(values, sum(values)) | ||
function $(esc(name))(values::AbstractVector{<:Real}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative that would be safer would be to define inner constructor:
macro weights(name)
return quote
mutable struct $(esc(name)){S<:Real, T<:Real, V<:AbstractVector{T}} <: AbstractWeights{S, T, V}
values::V
sum::S
function $(esc(name)){S, T, V}(values::AbstractVector{T}, s::S) where {S<:Real, T<:Real, V<:AbstractVector{T}}
Base.require_one_based_indexing(values)
return new(values, s)
end
end
$(esc(name))(values::AbstractVector{<:Real}, s=sum(values)) =
$(esc(name)){typeof(s), eltype(values), typeof(values)}(values, s)
end
end
but this looses all promotion rules that default inner constructor comes with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that we want to require 1-based indexing in all cases, the inner constructor approach sounds safer. How many methods do we need to define to replicate the default inner constructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have closed the PR since it seems we want to support non-1-based indexing. I think this issue should be settled first.
""" | ||
macro weights(name) | ||
return quote | ||
mutable struct $name{S<:Real, T<:Real, V<:AbstractVector{T}} <: AbstractWeights{S, T, V} | ||
values::V | ||
sum::S | ||
end | ||
$(esc(name))(values::AbstractVector{<:Real}) = $(esc(name))(values, sum(values)) | ||
function $(esc(name))(values::AbstractVector{<:Real}) | ||
Base.require_one_based_indexing(values) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This requires Julia >= 1.2: https://github.com/JuliaLang/julia/blob/8a0a7199ddc45ec70d8a0b74ab397ca69d34b50e/base/abstractarray.jl#L119-L120
The issue here is also fixed by the single line of code That fix is included in #722. Making We have to pick one of
I prefer option 3. |
I am OK to go for option 3 so I am closing this PR. |
Currently we have:
I propose to require that
AbstractWeights
take a vector using 1-based indexing. This is a safer option to the alternative, where we would recalculate indices everywhere (this is possible, but I think error prone, so it is better to be defensive)