-
Notifications
You must be signed in to change notification settings - Fork 16
Add basic vector transforms equivalent to existing scalar ones, along with vector transform composition #142
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
base: master
Are you sure you want to change the base?
Conversation
@tpapp thoughts on the current code in |
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 wonder if it would be better to define an alternative to ArrayTransformation
that transforms each element with a possibly different transformation.
struct VectorIdentity <: VectorTransform | ||
d::Int | ||
function VectorIdentity(d) | ||
new(d) | ||
end | ||
end |
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 transform already exists in a more general form: as(Real, dims...)
If there are performance improvements possible I think it would be better to implement them this existing array transformation.
struct VecTVExp <: VectorTransform | ||
d::Int | ||
function VecTVExp(d) | ||
new(d) | ||
end | ||
end |
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.
Same here, a more general version of this transform already exists.
struct VecTVLogistic <: VectorTransform | ||
d::Int | ||
function VecTVLogistic(d) | ||
new(d) | ||
end | ||
end |
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.
Same here, also this already exists.
""" | ||
$(TYPEDEF) | ||
|
||
Shift transformation `x ↦ x + shift`. | ||
""" | ||
struct VecTVShift{T<:Real} <: VectorTransform |
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.
One could exploit muladd
when not decomposing shifting and scaling (which in my experience are usually both needed) in separate steps.
|
||
dimension(t::VecTVShift) = length(t.shift) | ||
transform_with(flag::LogJacFlag, t::VecTVShift, x::AbstractVector{T}, index::Int) where {T} = x .+ t.shift, logjac_zero(flag, T), index + dimension(t) | ||
inverse_eltype(t::VecTVShift, T::Type) = eltype(T) |
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 must also take into account the element type of the shift. Moreover, currently the compiler won't specialize on T
.
inverse_eltype(t::VecTVShift, T::Type) = eltype(T) | |
inverse_eltype(t::VecTVShift, ::Type{<:AbstractVector{T}}) where {T<:Real} = promote_type(eltype(t.shift), T) |
Shift transformation `x ↦ x + shift`. | ||
""" | ||
struct VecTVShift{T<:Real} <: VectorTransform | ||
shift::AbstractVector |
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.
The type should be a type parameter T<:AbstractVector{<:Real}
.
function VecTVShift(val::Real, dim::Integer) | ||
return VecTVShift(repeat([val;], dim)) | ||
end |
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 should just be
function VecTVShift(val::Real, dim::Integer) | |
return VecTVShift(repeat([val;], dim)) | |
end | |
function VecTVShift(val::Real, dim::Integer) | |
return VecTVShift(fill(val, dim)) | |
end |
Ideally, though, I think users might want to use FillArrays here. To avoid additional dependencies and keep it more flexible, I'd remove this definition:
function VecTVShift(val::Real, dim::Integer) | |
return VecTVShift(repeat([val;], dim)) | |
end |
@devmotion yes, I'm aware some of those already existed, just that in terms of API, I thought that if I want to have a concise/easier way to write compositions of vector transformations then something like
would be preferable to
Haven't tested for performance differences, will check.
That would be a more general approach, yes, but unsure how error prone composition of such transformation would be. If I have |
Addresses #141, but not only for
TVShift
but others too, with the ultimate goal of adding vector transform composition.Initial PR is to kick off discussions about API, as working off of something should be easier. Also, initial PR does not contain vector transform compositions yet.