Skip to content

Conversation

@N5N3
Copy link
Contributor

@N5N3 N5N3 commented Mar 25, 2022

Close #229.

@hyrodium hyrodium self-requested a review March 25, 2022 16:11
@hyrodium
Copy link
Collaborator

Thanks for the contribution!
Btw, I'm not sure why I couldn't find the "approve and run" button here 🤔

image

I tried the tests on my local machine, and it thew the following error.

Test Summary: | Pass  Total
Util          |  201    201
Core: Test Failed at /home/hyrodium/.julia/dev/Rotations/test/2d.jl:21
  Expression: Angle2d((1, 0, 0, 1, 0))
    Expected: DimensionMismatch
      Thrown: ArgumentError
Stacktrace:
 [1] macro expansion
   @ ~/.julia/dev/Rotations/test/2d.jl:21 [inlined]
 [2] macro expansion
   @ ~/julia/julia-1.7.1/share/julia/stdlib/v1.7/Test/src/Test.jl:1283 [inlined]
 [3] macro expansion
   @ ~/.julia/dev/Rotations/test/2d.jl:11 [inlined]
 [4] macro expansion
   @ ~/julia/julia-1.7.1/share/julia/stdlib/v1.7/Test/src/Test.jl:1283 [inlined]
 [5] top-level scope
   @ ~/.julia/dev/Rotations/test/2d.jl:10
Test Summary:                    | Pass  Fail  Total
2d Rotations                     | 4864     1   4865
  Core                           |   10     1     11
  Unitful                        |    4            4
  Identity rotation checks       |   12           12
  zero checks                    |   18           18
  Testing inv()                  | 1000         1000
  Testing norm() and normalize() |  600          600
  Rotate Points                  |  202          202
  Rotate Unitful Points          |  606          606
  Compose rotations              |  800          800
  Convert rotations              |  200          200
  Types and products             |   12           12
  RotMatrix{2} vs Angle2d        |  400          400
  Angle2d                        |  700          700
  angle                          |  300          300
ERROR: LoadError: Some tests did not pass: 4864 passed, 1 failed, 0 errored, 0 broken.
in expression starting at /home/hyrodium/.julia/dev/Rotations/test/2d.jl:4
in expression starting at /home/hyrodium/.julia/dev/Rotations/test/runtests.jl:17
ERROR: Package Rotations errored during testing

This seems to be caused by StaticArrays throwing ArgumentError for SMatrix{2,2}(1,0,0,1,0). But I think this should be DimensionMismatch.

@N5N3
Copy link
Contributor Author

N5N3 commented Mar 25, 2022

Oops, I forgot to swith back to StaticArrays's master branch when testing this. (With JuliaArrays/StaticArrays.jl#1016, SMatrix{2,2}(1,0,0,1,0) throws a DimensionMismatch).
We could replace it with SArray{Tuple{2,2}} as, SArray{Tuple{2,2}}(1,2,3,4,5) throws a DimensionMismatch.

@codecov
Copy link

codecov bot commented Mar 26, 2022

Codecov Report

Merging #230 (fc774f1) into master (a12cb6c) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #230   +/-   ##
=======================================
  Coverage   88.45%   88.45%           
=======================================
  Files          16       16           
  Lines        1602     1602           
=======================================
  Hits         1417     1417           
  Misses        185      185           
Impacted Files Coverage Δ
src/core_types.jl 95.62% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

Copy link
Collaborator

@hyrodium hyrodium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
After the last commit & push, the "approve and run" button has appeared.

@hyrodium hyrodium merged commit ecc6595 into JuliaGeometry:master Mar 26, 2022
@N5N3 N5N3 deleted the enable_promote branch March 27, 2022 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RotMatrix(x::Tuple) might return weird result if !isa(x, NTuple)

2 participants