1-update syntax julia1.6.2 #2

gdelvecchiodelvecchio · 2021-08-28T15:58:23Z

Updated syntax for Julia >1.6.2. Tests were failing with integer valued matrices but the problem is in PFAPACK python implementation. Will open a pull request there too. Tests fail with complex valued matrices because of an assertion error in PFAPACK and I am proceeding with closer investigation

…y integers

…ecchiodelvecchio/Pfaffian.jl into 1-update-syntax-julia1.6.2

mishmash · 2021-09-14T23:52:44Z

Thanks for this. This thing is indeed in desperate need of an update...

So I installed the latest pfapack via conda and built PyCall using that environment. However, upon running the tests, I get the following error involving pfapack:

N = 100

Testing real methods:

maximum(abs, v - v_w) = 1.1102230246251565e-16
abs(tau - tau_w) = 0.0
abs(alpha - alpha_w) = 8.881784197001252e-16

maximum(abs, T - T_w) = 1.021405182655144e-14
maximum(abs, Q - Q_w) = 6.147859998861804e-15

abs((pf_ltl - pf_ltl_w) / pf_ltl_w) = 8.762739807355889e-15

abs((pf_h - pf_h_w) / pf_h_w) = 1.1482210782052687e-14


Testing integer methods:

maximum(abs, v - v_w) = 0.0
abs(tau - tau_w) = 0.0
abs(alpha - alpha_w) = 0.0

maximum(abs, T - T_w) = 2.6290081223123707e-13
maximum(abs, Q - Q_w) = 1.725009024511337e-14

ERROR: LoadError: PyError ($(Expr(:escape, :(ccall(#= /Users/mishmash/.julia/packages/PyCall/BD546/src/pyfncall.jl:43 =# @pysym(:PyObject_Call), PyPtr, (PyPtr, PyPtr, PyPtr), o, pyargsptr, kw))))) <class 'TypeError'>
TypeError('No loop matching the specified signature and casting was found for ufunc true_divide')
  File "/Users/mishmash/opt/anaconda3/envs/py37/lib/python3.7/site-packages/pfapack/pfaffian.py", line 245, in pfaffian
    return pfaffian_LTL(A, overwrite_a)
  File "/Users/mishmash/opt/anaconda3/envs/py37/lib/python3.7/site-packages/pfapack/pfaffian.py", line 298, in pfaffian_LTL
    tau /= A[k, k + 1]

Stacktrace:
  [1] pyerr_check
    @ ~/.julia/packages/PyCall/BD546/src/exception.jl:62 [inlined]
  [2] pyerr_check
    @ ~/.julia/packages/PyCall/BD546/src/exception.jl:66 [inlined]
  [3] _handle_error(msg::String)
    @ PyCall ~/.julia/packages/PyCall/BD546/src/exception.jl:83
  [4] macro expansion
    @ ~/.julia/packages/PyCall/BD546/src/exception.jl:97 [inlined]
  [5] #107
    @ ~/.julia/packages/PyCall/BD546/src/pyfncall.jl:43 [inlined]
  [6] disable_sigint
    @ ./c.jl:458 [inlined]
  [7] __pycall!
    @ ~/.julia/packages/PyCall/BD546/src/pyfncall.jl:42 [inlined]
  [8] _pycall!(ret::PyObject, o::PyObject, args::Tuple{Matrix{Int64}}, nargs::Int64, kw::PyObject)
    @ PyCall ~/.julia/packages/PyCall/BD546/src/pyfncall.jl:29
  [9] _pycall!
    @ ~/.julia/packages/PyCall/BD546/src/pyfncall.jl:11 [inlined]
 [10] (::PyObject)(args::Matrix{Int64}; kwargs::Base.Iterators.Pairs{Symbol, String, Tuple{Symbol}, NamedTuple{(:method,), Tuple{String}}})
    @ PyCall ~/.julia/packages/PyCall/BD546/src/pyfncall.jl:86
 [11] top-level scope
    @ ~/Dropbox/GitHub/Pfaffian.jl/test-Pfaffian.jl:79
in expression starting at /Users/mishmash/Dropbox/GitHub/Pfaffian.jl/test-Pfaffian.jl:79

mishmash · 2021-09-15T01:24:06Z

Never mind. It looks like your recent PR in pfapack is not in the release version there yet. It's been merged to master but looks like there are still outstanding issues? I'm a bit confused though about when/how the issues with pfapack cropped up. Is it something specific to the PyPI/GitHub version that is not present in Wimmer's original code (https://michaelwimmer.org/downloads.html) that I was using originally?

gdelvecchiodelvecchio · 2021-09-15T07:59:02Z

Good morning, The pull request should have been merged as you say I don't really understand. That kind of error is exactly the one that I fixed. Let me check on PFAPACK github rep I will get back to you! Thanks a lot! Giuseppe

…

On Wed, 15 Sep 2021, 03:24 Ryan Mishmash, ***@***.***> wrote: Never mind. It looks like your recent PR <basnijholt/pfapack#4> in pfapack is not in the release version there yet. It's been merged to master but looks like there are still outstanding issues? I'm a bit confused though about when/how the issues with pfapack cropped up. Is it something specific to the PyPI/GitHub version that is not present in Wimmer's original code ( https://michaelwimmer.org/downloads.html) that I was using originally? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AH4RLYAOJXBSZYN6E4WRXVDUB7YUDANCNFSM5C7H5KWQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

mishmash · 2021-09-15T18:48:42Z

Yeah, it looks like your PR was merged, but there are still some failing tests (see comments in basnijholt/pfapack#4), so the changes haven't made it to the released PyPI version yet (the latest of which is from March 12, 2020); thus, things will still fail with a conda installed pfapack. @basnijholt, any updates on that?

basnijholt · 2021-09-16T10:27:28Z

@mishmash, right. The tests that @gdelvecchiodelvecchio added seem to fail.

I have currently have no time to look at it. It would be great if @gdelvecchiodelvecchio could reply to my comments here basnijholt/pfapack#4 (comment).

When these issues are resolved I can release a new version.

basnijholt · 2021-09-17T17:20:31Z

@mishmash and @gdelvecchiodelvecchio, I have released v0.3.1: https://github.com/basnijholt/pfapack/releases/tag/v0.3.1

mishmash · 2021-09-20T17:51:57Z

test-Pfaffian.jl


 T, Q = skew_tridiagonalize(A)
-T_w, Q_w = pfaffian[:skew_tridiagonalize](A)
+T_w, Q_w = pf.skew_tridiagonalize(A)


This currently throws

File "/Users/mishmash/opt/anaconda3/envs/pfaffiantests37/lib/python3.7/site-packages/pfapack/pfaffian.py", line 93, in skew_tridiagonalize assert abs((A + A.T).max()) < 1e-14

since A' is conjugate transpose. Should probably use transpose throughout to make the antisymmetric matrices.

And also in the corresponding antisymmetric-checking asserts in the Julia code.

@mishmash do you mean in the corresponding Julia code right? If so yes, you should use transpose(A) and not the ' as it does the conjugation too.

Yes, in the Julia code we need to be careful about transpose vs. conjugate transpose throughout.

mishmash · 2021-09-23T22:38:54Z

Pfaffian.jl

@@ -48,7 +50,7 @@ end

 function skew_tridiagonalize(A::Matrix{T}; overwrite_A=false, calc_Q=true) where {T <: Number}
    @assert size(A)[1] == size(A)[2] > 0
-    @assert maximum(abs, A + A.') < 1e-12
+    @assert maximum(abs, A + A') < 1e-12


Change to transpose.

mishmash · 2021-09-23T22:39:17Z

Pfaffian.jl

    @assert size(A)[1] == size(A)[2] > 0
-    @assert maximum(abs, A + A.') < skewsymtol
+    @assert maximum(abs, A + A') < skewsymtol


Change to transpose.

mishmash · 2021-09-23T22:39:32Z

Pfaffian.jl

    @assert size(A)[1] == size(A)[2] > 0
-    @assert maximum(abs, A + A.') < skewsymtol
+    @assert maximum(abs, A + A') < skewsymtol


Change to transpose.

mishmash · 2021-09-23T22:40:25Z

test-Pfaffian.jl


 @show maximum(abs, v - v_w)
 @show abs(tau - tau_w)
 @show abs(alpha - alpha_w)
 println()

 A = rand(Float64, (N, N))
-A = A - A.'
+A = A - A'


Change to transpose.

mishmash · 2021-09-23T22:40:44Z

test-Pfaffian.jl

+
+
+A = rand(-10:10, (N, N))
+A = A - A'


Change to transpose.

mishmash · 2021-09-23T22:41:05Z

test-Pfaffian.jl

-A = rand(Complex128, (N, N))
-A = A - A.'
+A = rand(ComplexF64, (N, N))
+A = A - A'


Change to transpose.

gdelvecchiodelvecchio added 3 commits August 28, 2021 15:18

updated syntax, updated tests, still not working with matrices made b…

fa8d5cf

…y integers

updated syntax julia 1.6.2, updated tests

c021c71

Merge branch '1-update-syntax-julia1.6.2' of https://github.com/gdelv…

3acdb23

…ecchiodelvecchio/Pfaffian.jl into 1-update-syntax-julia1.6.2

mishmash reviewed Sep 20, 2021

View reviewed changes

mishmash reviewed Sep 23, 2021

View reviewed changes

test-Pfaffian.jl

A = rand(-10:10, (N, N))

A = A - A'

Copy link

Owner

mishmash Sep 23, 2021

Choose a reason for hiding this comment

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

Change to transpose.

mishmash reviewed Sep 23, 2021

View reviewed changes

test-Pfaffian.jl

A = rand(Complex128, (N, N))

A = A - A.'

A = rand(ComplexF64, (N, N))

A = A - A'

Copy link

Owner

mishmash Sep 23, 2021

Choose a reason for hiding this comment

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

Change to transpose.

1-update syntax julia1.6.2 #2

Are you sure you want to change the base?

1-update syntax julia1.6.2 #2

Uh oh!

Conversation

gdelvecchiodelvecchio commented Aug 28, 2021

Uh oh!

mishmash commented Sep 14, 2021

Uh oh!

mishmash commented Sep 15, 2021

Uh oh!

gdelvecchiodelvecchio commented Sep 15, 2021 via email

Uh oh!

mishmash commented Sep 15, 2021

Uh oh!

basnijholt commented Sep 16, 2021

Uh oh!

basnijholt commented Sep 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!