CRP and MRP implementation

[tahoe-vegas]

Added Classic and Modified Rodrigues parameters as new attitude representation types CRP and MRP with similar methods as the other existing types

[ronisbr]

Awesome! Thanks @tahoe-vegas !

[Copilot]

Pull request overview

This PR implements two new attitude representation types: Classical Rodrigues Parameters (CRP) and Modified Rodrigues Parameters (MRP). These are minimal 3-parameter representations of rotations that complement the existing types (DCM, EulerAngles, EulerAngleAxis, and Quaternion) in the ReferenceFrameRotations.jl package.

Changes:

• Added CRP and MRP type definitions with full API support including constructors, arithmetic operations, composition, conversions, and kinematics
• Implemented comprehensive conversion functions between CRP/MRP and existing rotation types (DCM, Quaternion, EulerAngles, EulerAngleAxis)
• Added shadow rotation functionality specific to MRP (identity for CRP)

Reviewed changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/types.jl	Defines CRP and MRP struct types with 3 components (q1, q2, q3) and adds them to ReferenceFrameRotation union
src/crp.jl	Complete CRP implementation: constructors, Julia API, kinematics (dcrp), I/O, operations, and utility functions
src/mrp.jl	Complete MRP implementation: constructors, Julia API, kinematics (dmrp), I/O, operations, and utility functions
src/shadow_rotation.jl	Implements shadow rotation for MRP (switches to alternate representation) and CRP (identity)
src/random.jl	Adds random sampling support for CRP and MRP via quaternion conversion
src/inv_rotations.jl	Extends inv_rotation function to support CRP and MRP
src/compose_rotations.jl	Adds compose_rotation implementations for CRP and MRP
src/conversions/*.jl	Conversion functions between CRP/MRP and other rotation types (8 new files)
src/conversions/api.jl	Extends Base.convert API to support CRP and MRP conversions
src/ReferenceFrameRotations.jl	Includes new source files and conversion functions
test/crp.jl	Comprehensive tests for CRP functionality including constructors, conversions, operations, kinematics
test/mrp.jl	Comprehensive tests for MRP functionality including constructors, conversions, operations, kinematics, shadow rotation
test/runtests.jl	Integrates CRP and MRP test suites into main test runner

Comments suppressed due to low confidence (13)

src/crp.jl:220

• The singularity check uses a hardcoded tolerance of 1e-15 and throws an ArgumentError. This tolerance should either be documented or made configurable. Additionally, the error message could be more specific about which inputs caused the singularity. Consider including the actual dot product value in the error message to help with debugging.

    if isapprox(norm_c1_c2, 1; atol = 1e-15)
        # TODO: Return a specific error?
        throw(ArgumentError("The composition of these CRPs results in a specific singularity (180 degrees rotation)."))
    end

src/inv_rotations.jl:77

• Missing test coverage: The inv_rotations.jl test file should include tests for CRP and MRP to maintain consistency with the testing pattern for DCM, Quaternion, EulerAngleAxis, and EulerAngles. The test/crp.jl and test/mrp.jl files test the inv() function but not the inv_rotation() wrapper function.

@inline inv_rotation(q::CRP) = inv(q)
@inline inv_rotation(s::MRP) = inv(s)

test/mrp.jl:155

• Inconsistent naming in comment: The comment refers to "MRP q" but should be "MRP m" to match the variable naming convention used throughout the codebase.

    # The inverse of a MRP q is -q

test/crp.jl:155

• Inconsistent naming in comment: The comment refers to "CRP q" but should be "CRP c" to match the variable naming convention used throughout the codebase.

    # The inverse of a CRP q is -q

src/inv_rotations.jl:20

• Spelling error: "anlges" should be "angles".

- A set of Euler anlges (`EulerAngles`);

src/inv_rotations.jl:77

• Inconsistent parameter naming: the parameter for CRP is named 'q' but should be 'c' to match the convention used throughout the codebase where CRP instances are named 'c'. The parameter for MRP is named 's' but should be 'm' to match the convention used elsewhere (e.g., in shadow_rotation.jl and the conversion functions).

@inline inv_rotation(q::CRP) = inv(q)
@inline inv_rotation(s::MRP) = inv(s)

test/crp.jl:211

• The test for shadow_rotation(c) is incomplete. The test only checks that c == c_shadow, which is expected since shadow_rotation for CRP returns the input unchanged. However, the test should verify this is the intended behavior rather than just asserting equality. Consider adding a comment explaining why CRP shadow rotation is identity, or add a more meaningful test.

@testset "CRP Shadow Rotation" begin
    c = CRP(1.0, 2.0, 3.0)
    c_shadow = shadow_rotation(c)
    
    @test c == c_shadow
end

src/conversions/quat_to_mrp.jl:15

• The comment block (lines 9-15) contains excessive commentary and thought process that should be removed or condensed. Comments should be concise and explain the final decision, not the reasoning process. Consider replacing with a single line like: "MRP is singular at 360-degree rotations (q0 = -1)".

    # Check for singularity: q0 = -1 (360 degrees rotation, which is 0 mod 360, but MRP singularity is at 360? 
    # Wait, MRP singularity is at +/- 360 degrees (q0 = -1).
    # Normal MRP is singular at +/- 360 deg?
    # No, MRP is singular at +/- 360 degrees (4*arctan(sigma)).
    # sigma = tan(Phi/4). Phi = 360 -> tan(90) = inf.
    # q0 = cos(Phi/2) = cos(180) = -1.
    # So singularity is at q0 = -1.

src/crp.jl:218

• TODO comment should be resolved or removed before merging. Either implement a specific error type for singularity or remove the TODO comment and document why ArgumentError is appropriate.

        # TODO: Return a specific error?

src/random.jl:86

• Missing test coverage: The random.jl test file should include tests for CRP and MRP random generation to maintain consistency with the testing pattern for other rotation types (DCM, EulerAngleAxis, EulerAngles, Quaternion). While test/crp.jl and test/mrp.jl include basic tests for rand(), they don't use StableRNG for reproducibility testing.

# == CRP ===================================================================================

function Random.rand(rng::AbstractRNG, ::Random.SamplerType{R}) where R <: CRP
    T = eltype(R)
    if T == Any
        T = Float64
    end

    return quat_to_crp(_rand_quat(rng, T))
end

# == MRP ===================================================================================

function Random.rand(rng::AbstractRNG, ::Random.SamplerType{R}) where R <: MRP
    T = eltype(R)
    if T == Any
        T = Float64
    end

    return quat_to_mrp(_rand_quat(rng, T))
end

test/mrp.jl:63

• Inconsistent capitalization: "Are" should be lowercase "are" in the middle of the sentence.

    # Signs of quaternions can be flipped, so check if they Are approximately equal or opposite

test/crp.jl:63

• Inconsistent capitalization: "Are" should be lowercase "are" in the middle of the sentence.

    # Signs of quaternions can be flipped, so check if they Are approximately equal or opposite

src/compose_rotations.jl:98

• Missing test coverage: The compose_rotations.jl test file should include tests for CRP and MRP compose_rotation functions to maintain consistency with the testing pattern for DCM, EulerAngleAxis, EulerAngles, and Quaternion. While the CRP and MRP test files include basic compose_rotation tests, the comprehensive multi-argument tests in compose_rotations.jl are not included for these new types.

@inline compose_rotation(c::CRP) = c
@inline compose_rotation(c::CRP, cs::CRP...) = compose_rotation(cs...) * c

@inline compose_rotation(m::MRP) = m
@inline compose_rotation(m::MRP, ms::MRP...) = compose_rotation(ms...) * m

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.