Fix and test edge case for matrix conversion

[tungalbert99]

There's a slight edge case I found due to numerical instability where the matrix conversion will error for arccosine if the value is 1.0 + epsilon and provide a NaN value. It will trigger specifically the code path in lines 995-996 but I have not found an orientation that triggers 997-998. If you want, I can remove that code path or add a corresponding test.

[AlexanderFabisch]

Hi @tungalbert99 ,

thanks for pointing this out! I added two comments on the format. Then we can merge it to master with the next release of the library.

[AlexanderFabisch]

Thanks. I would like to keep 100% coverage. Do you have an idea of how we could find a test case for the missing branch?

[AlexanderFabisch]

I ignore the branch for coverage computation for now, but this is not a good solution. The pull request has been merged to develop and this will be part of the next release. Thanks for the contribution!

[AlexanderFabisch]

@tungalbert99 I looked at the fix again and I changed it to this solution. Was there a reason why you set values close to 0 to exactly 0? It does not seem to make sense in my opinion. Forcing an argument of arccos to be within [-1, 1] does make sense to me intuitively, of course.

[tungalbert99]

Sorry, I think I had mistaken the domain for arccosine when I wrote this. The reason I didn't clip the argument was that if it exceeded the bounds of [-1,1] by more than epsilon (absolute diff), then it's likely that an invalid rotation was passed in? I'm not sure how valid that conjecture is though.

[AlexanderFabisch]

We check the correctness of the rotation matrix in the first line of the function already. So I will just clip the values. Thanks for clarification!

[tungalbert99]

Awesome! Thanks for maintaining an incredibly useful library

[AlexanderFabisch]

Thanks a lot! :)