v2.1 Revision

[MicheleCeresoli]

This is a list of points, in no particular order, that I think needs either fixing or minor changes:

Proposed:

• [x] Directions should be normalised. I believe returning non-unit vectors from the direction routines could lead to many mistakes. Although this change might limit the total use cases, I think there are few applications in which one would benefit from having non-unit directions.

• [ ] What's the purpose of the FramePointFunctions{T, O}(fun::Function) and FrameAxesFunctions{O, N}(fun::Function)? In my understanding, the only case in which you would want to use the same functions across all orders is for fixed points, axes or directions. However, they are not implemented in the corresponding functions.

• [ ] Fixed offset axes being registered as inertial is a mistake because they could be fixed with respect to a set rotating axes, making them non-inertial.

• [ ] I would remove add_axes_root replacing it with add_axes_inertial, as it was in the previous release.

• [ ] I don't agree on having add_axes_inertial require a function with respect to time to compute their orientation. I would re-implement the previous distinction between inertial and projected axes, having add_axes_inertial take as input a DCM and add_axes_projected behaving as the current add_axes_inertial.

• [x] Add an additional constructor for Rotation, such that Rotation{O}(R:Rotation{O}) = R, to avoid useless conversions in the transform functions.

• [x] I would use T as the default type letter to indicate the Type of the argument, as it is done in the entire ecosystem. In this version, there are a number of letter changes between type definitions that make code readability hard. For example in ad.jl types are defined with the letter T, whilst in nodes.jl they use the letter N.

• [ ] I believe the ForwardDiff APIs implementation for the Rotation object has been left out from this release (it was available in the previous one).

• [ ] The SVectorNT constructor should be changed. At the moment, since it is a type alias of SVector it introduces two additional constructors on top of StaticArray's original implementation. However, since that is one of the most used packages in the Julia ecosystem, it could cause undesired behaviours and errors.

• [ ] I am not a complete fan of the new ways a lot of constructors (e.g., in Rotation) have been re-written. I don't believe that manually building expressions in every scenario actually helps readability of what's going on.

I'm marking as accepted all the proposed changes that we agree would need to be implemented for the next release.

@andreapasquale94 what do you think?

[andreapasquale94]

Fixed offset axes being registered as inertial is a mistake because they could be fixed with respect to a set rotating axes, making them non-inertial.

Agreed. I actually would remove completely the axes type.. remained there for "legacy" and it doesn't bring anything as is on the user to properly use axes.

What's the purpose of the FramePointFunctions{T, O}(fun::Function) and FrameAxesFunctions{O, N}(fun::Function)? In my understanding, the only case in which you would want to use the same functions across all orders is for fixed points, axes, or directions. However, they are not implemented in the corresponding functions.

True, as we use the generic axes/points builder we can remove them. As far as we find another way of avoiding closures for this:

@generated function FrameAxesFunctions{O, N}(fun::Function) where {O, N}
    expr = Expr(:call, :tuple)
    for i in 1:O 
        push!(
            expr.args,
            Expr(
                :call,
                Expr(:curly, :FrameAxesFunWrapper, O, N),
                :fun
            )
        )
    end
    pexpr = Expr(
        :call, 
        Expr(:curly, :FrameAxesFunctions, O, N, 3O),
        expr
    )

    return quote
        @inbounds $(pexpr)
    end
end

function FrameAxesFunctions{O, N}() where {O, N}
    return FrameAxesFunctions{O, N}(t->Rotation{O, N}(N(1)I))
end

I would remove add_axes_root replacing it with add_axes_inertial, as it was in the previous release.

Agreed. That was somehow a placeholder method, we can keep the previous approach.

I believe the ForwardDiff APIs implementation for the Rotation object has been left out from this release (it was available in the previous one).

Yes, this was intentional. I would prefer to open a PR dedicated to AD only.

The SVectorNT constructor should be changed. At the moment, since it is a type alias of SVector it introduces two additional constructors on top of StaticArray's original implementation. However, since that is one of the most used packages in the Julia ecosystem, it could cause undesired behaviours and errors.

I agree, as we discussed, I would favor a re-introduction of Translations as an intermediate type here.

I am not a complete fan of the new ways a lot of constructors (e.g., in Rotation) have been re-written. I don't believe that manually building expressions in every scenario actually helps readability of what's going on.

That was an attempt to uniform the way of writing generated functions, as I'm not a fan of the "mixed" notation. After one got used to Expr, I think the readability is good enough.