Acos in matrix angle

[bdeket]

possible alternative for fixing acos/asin (#99 )

• restrict type for matrix-basis-angle based on corrected type of acos/asin (@samth ) (which errors since it depends on matrix-basis-cos-angle which is not implemented)
• improve matrix-cos-angle so
  • Real values are between -1 and 1
  • Complex values have a magnitude at most (flnext 1.)
  • prevent early under/overflow
  • rescale matrix with inf values to a matrix of zero and ones (so it works similar to angle)
• improve matrix-angle so original type can be preserved
  • For real values use flacos
  • ok since Real values are now guaranteed to be within -1 to 1

[samth]

Does this type check with the revised Typed Racket?

[bdeket]

It should (I think), while preparing this I shadowed acos with one with the type:

(case->
 (-> One Zero)
 (-> Float-Complex Float-Complex)
 (-> Single-Flonum-Complex Single-Flonum-Complex)
 (-> Inexact-Complex Inexact-Complex)
 (-> Number Number))

But I did not test both together. Will try to do that tomorrow.

[bdeket]

It seems to work as I wanted:

Welcome to DrRacket, version 8.16.0.1 [3m].
Language: typed/racket/base [custom]; memory limit: 8192 MB.
> (require math)
  (:print-type acos)
  (:print-type matrix-angle)
  (matrix-angle (matrix [[5. 6.]])
                (matrix [[2. 8.]]))
; (case->
;  (-> One Zero)
;  (-> Float-Complex Float-Complex)
;  (-> Single-Flonum-Complex Single-Flonum-Complex)
;  (-> Inexact-Complex Inexact-Complex)
;  (-> Number Number))
; (case->
;  (-> (Array Flonum) (Array Flonum) Flonum)
;  (-> (Array Real) (Array Real) Real)
;  (-> (Array Float-Complex) (Array Float-Complex) Float-Complex)
;  (-> (Array Number) (Array Number) Number))
; - : Flonum
; 0.4497596130698395

[samth]

The math library successfully compiles for me with this change. Should we merge?

[mfelleisen]

I am okay with merging.

[samth]

@bdeket is this ready to merge? @soegaard or @pavpanchekha, any thoughts?

[bdeket]

Hi,
yes in my opinion it is ready. But I was hoping someone else would have a closer look at it before merging.

[soegaard]

Looks good to me.

Btw - in

(define (mag² [x : Number]) (if (real? x) (sqr x) (+ (sqr (real-part x)) (sqr (imag-part x)))))

why not use (magnitude x) ?

[bdeket]

Thanks
Re mag²:

1. To avoid an unnecesary (sqr (sqrt ... )) since we need (sqr (magnitude ...))
2. (sqr (magnitude 1.819465078987554e+98-1.7785942329203636e+101i)) and numbers in this region result in an flulp-error of ~2.1, whereas avoiding the (sqr (sqrt ...)) has flulp-error < 1 in my random testing (... on windows :/ which might be machine specific...)

[samth]

@bdeket this broke the http://drdr.racket-lang.org/68985/cs/racket/share/pkgs/math-test/math/tests/matrix-tests.rkt test. Can you take a look?

[bdeket]

I Tested again on my (win) computer and for me all tests pass.
I will try tomorrow on a linux machine.