add pinv to lubeck2 and make svd nothrow by aferust · Pull Request #55 · kaleidicassociates/lubeck

aferust · 2024-05-10T09:31:37Z

pinv is added to lubeck2. for potential code reviewers, please confirm this:

instead of just enforce!("svd: " ~ msg)(!info);

I had to use the below version to make the function nothrow. I am using this method in DCV a lot. It might be a problem for release builds due to assert, but at least it is nothrow, and works with debug builds.

try enforce!("svd: " ~ msg)(!info);
catch(Exception e) assert(false, e.msg);

aferust · 2024-05-10T09:33:05Z

@9il please review this.

9il · 2024-05-10T10:59:09Z

-        enforce!("svd: " ~ msg)(!info);
+
+        try enforce!("svd: " ~ msg)(!info);
+        catch(Exception e) assert(false, e.msg);


SVD isn't supposed to be nothrow with the current API. You may want to add an additoinal low-level API that will incorporate the info into the result.

9il · 2024-05-10T10:59:17Z

+{
+    auto matrixScope = matrix.lightScope.lightConst;
+    return pinv(matrixScope, tolerance); 
+}


please add a test

9il · 2024-05-10T11:06:55Z

What is wrong with mldivide?

9il · 2024-05-10T11:10:27Z

What is wrong with mldivide?

Ah, I see, mldivide will require identity matrix allocation. OK then

aferust · 2024-05-10T11:16:37Z

Hello, @9il, I am adapting an image stitching method (ransac homography estimation) DCV. I want to keep nogc nothrow with DCV. So, if we cannot have a function pinv nogc nothrow, I cannot do that. I have already tested my modification and I can do some elegant panaroma stitching. But yes I will add a unittest. If we don't have a nothrow solution, I will use my fork as a dependency?

9il · 2024-05-10T11:19:46Z

You didn't understand me. I suggest you to add an additional low-level API that will be ~~throw~~ nothrow and will incorporate the info, rework pinv based on this API, call this API inside the old API. It is a simple nothrow.

9il · 2024-05-10T11:20:48Z

damn, autocorrection replaces nothrow with throw

aferust · 2024-05-10T11:25:26Z

Ok, now I am working on the RANSAC thing. When I find some time, I will review the "info" and try to be more familiar with it and add an additoinal low-level API as you suggest. Until it, let's keep this PR open.

aferust · 2024-05-10T11:44:07Z

just to see what I was trying to do: https://github.com/aferust/dcv/tree/master/examples/imagestitchinghomography

aferust · 2024-05-10T12:21:48Z

You didn't understand me. I suggest you to add an additional low-level API that will be ~~throw~~ nothrow and will incorporate the info, rework pinv based on this API, call this API inside the old API. It is a simple nothrow.

do you mean creating a nothrow duplicate of svd and call it inside the pinv?

jmh530 · 2024-05-10T14:15:07Z

+    else
+        auto si = st[0];
+    auto s = svd.sigma[0 .. $ - si];
+    s.each!"a = 1 / a";


I haven't tried this code or anything, but I would definitely want to make sure this line shouldn't be something like s.each!"a = 1.0 / a"

1 / is fine. It can be 1f / but 1.0 / . The last one will trigger float to double conversion.

I agree 1f is better. I was a little concerned because the function signature doesn't constrain T to be floating point (though I imagine some of the other functions called make that restriction, it isn't immediately obvious from looking at this one if that is the case).

9il · 2024-05-10T15:38:15Z

You didn't understand me. I suggest you to add an additional low-level API that will be ~~throw~~ nothrow and will incorporate the info, rework pinv based on this API, call this API inside the old API. It is a simple nothrow.

do you mean creating a nothrow duplicate of svd and call it inside the pinv?

Yes. Please avoid code duplication. The old API should reuse the new one nothrow API.

aferust · 2024-05-10T15:44:40Z

I understood. I will make the required changes when i am available.

aferust · 2024-05-10T18:52:18Z

Before starting to do anything, I have just used a test code to determine an assertion workflow, but without touching anything, actually running on run.dlang.org the second assert fails. is it normal:

/+dub.sdl:
dependency "lubeck" version="~>1.5.4"
+/
import std.stdio;
import mir.ndslice, mir.rc;

void main()
{
    import kaleidic.lubeck;
    
    import std.math : isClose;
    auto A = iota(15).as!double.sliced(3,5) + 1;

    auto A_pinv = pinv(A);

    auto A1 = mtimes(A, mtimes(A_pinv, A));
    auto A2 = mtimes(A_pinv, mtimes(A, A_pinv));

    auto boolMat1 = zip(A,      A1).map!((a,b) => a.isClose(b));
    auto boolMat2 = zip(A_pinv, A2).map!((a,b) => a.isClose(b));
    
    // for potential asserts
    boolMat1.all!(aa => aa == true).writeln; // true
    boolMat2.all!(aa => aa == true).writeln; // false ? only one value is different
	
    A_pinv.writeln;    
    A2.writeln;
}

outputs:
true
false
[[-0.246667, -0.0666667, 0.113333], [-0.133333, -0.0333333, 0.0666667], [-0.02, 3.77909e-18, 0.02], [0.0933333, 0.0333333, -0.0266667], [0.206667, 0.0666667, -0.0733333]]
[[-0.246667, -0.0666667, 0.113333], [-0.133333, -0.0333333, 0.0666667], [-0.02, -2.54426e-18, 0.02], [0.0933333, 0.0333333, -0.0266667], [0.206667, 0.0666667, -0.0733333]]

jmh530 · 2024-05-10T19:18:50Z

Try this:

/+dub.sdl:
dependency "lubeck" version="~>1.5.4"
+/
import kaleidic.lubeck;

void main()
{
    import mir.algorithm.iteration: equal;
    import mir.math.common: approxEqual;
    import mir.ndslice.slice: sliced;
    import mir.ndslice.topology: iota, as;

    auto A = iota([15], 1).as!double.sliced(3,5);

    auto A_pinv = pinv(A);

    auto A1 = mtimes(A, mtimes(A_pinv, A));
    auto A2 = mtimes(A_pinv, mtimes(A, A_pinv));

    assert(A.equal!approxEqual(A1));
    assert(A_pinv.equal!approxEqual(A2));
}

aferust · 2024-05-10T20:30:18Z

@9il , @jmh530 , please review my last commit.

9il · 2024-05-10T22:35:14Z

+)(
+    auto ref const Slice!(const(T)*, 2, kind) a,
+    out size_t infoResult,
+    out bool lowApiExecuted,


why this flag is needed?

with the original code, enforce was only triggered in the else block of if (m == 0 || n == 0), meaning that any lapack function ran. With that flag it provides the same logic. I was not going to put that flag. Then that idea seemed meaningful.

either way we check info. Do you think the flag is redundant?

Already removed

9il · 2024-05-11T10:59:47Z

+    if(lowApiExecuted)
+    {
+        enum msg ="pinv: pinv was not successful due to a convergence issue during SVD calculation.";
+        assert(!info,  msg);


Do you need convergence validation for pinv function?
If so, then please do the same: two implementations of pinv, where one of them is has the info out argument.

9il · 2024-05-11T11:00:22Z

+    return svdresult; //transposed
+}
+
+@safe pure @nogc nothrow SvdResult!T svdImpl


I think just svd name overload is OK.

aferust · 2024-05-11T11:04:44Z

I understood, but I am outside now. I do it when İ get home 11 May 2024 Cmt 14:00 tarihinde Ilia Ki ***@***.***> şunu yazdı:

…

***@***.**** commented on this pull request. ------------------------------ In source/kaleidic/lubeck2.d <#55 (comment)> : > @@ -1228,6 +1228,28 @@ Returns: error code from CBlas ) if (algorithm == "gesvd" || algorithm == "gesdd") { + + size_t info; + auto svdresult = svdImpl!(T, algorithm, kind)(a, info, slim); + + enum msg = (algorithm == "gesvd" ? "svd: DBDSDC did not converge, updating process failed" : "svd: DBDSQR did not converge"); + enforce!("svd: " ~ msg)(!info); + + return svdresult; //transposed +} + ***@***.*** pure @nogc nothrow SvdResult!T svdImpl I think just svd name overload is OK. — Reply to this email directly, view it on GitHub <#55 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACLDF6IGJM3NPUSJXCH3XK3ZBX25XAVCNFSM6AAAAABHQJZQR2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANJRGE4DCNJWGY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

aferust · 2024-05-11T14:02:58Z

@9il I have added some pinv overloads. They cover possible situations. And there are no signature collisions.

9il · 2024-05-12T13:15:02Z

Folks, it seems I no longer have write access to Lubeck. End of story.

aferust · 2024-05-13T11:20:55Z

Folks, it seems I no longer have write access to Lubeck. End of story.

I am sorry to hear it. I hope things positively change over time.

jmh530 · 2024-05-13T13:40:08Z

@9il It's boost licensed. Would you want to fork it and do new work there?

9il · 2024-05-14T08:21:40Z

I prefer not to maintain the repository. I still have the code.dlang.org name lubeck.

First, we need to ask @Laeeth, if and where he wishes me to transfer the ownership of the lubeck package name on code.dlang.org. We will need to wait a while to get a response.

The safest thing for now is for @jmh530 or @aferust to create a fork with another package name and all credentials and links to this repository.

jmh530 · 2024-05-14T15:57:44Z

@9il I have a fork already. It doesn't let you fork it twice.

add pinv to lubeck2 and make svd nothrow

5a15ac5

9il reviewed May 10, 2024

View reviewed changes

jmh530 reviewed May 10, 2024

View reviewed changes

aferust added 3 commits May 10, 2024 23:15

separate nothrow low level api for svd

5583077

fix msg

8e10b4c

add unittest for pinv

0fba3d7

fix return value of svd

2bf5e54

9il reviewed May 10, 2024

View reviewed changes

static if does not create a scope. and remove the lowapiexec flag

508a25a

9il reviewed May 11, 2024

View reviewed changes

some pinv overloads

455dd4d

9il approved these changes May 11, 2024

View reviewed changes

Conversation

aferust commented May 10, 2024

Uh oh!

aferust commented May 10, 2024

Uh oh!

9il May 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

9il commented May 10, 2024

Uh oh!

9il commented May 10, 2024

Uh oh!

aferust commented May 10, 2024

Uh oh!

9il commented May 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

9il commented May 10, 2024

Uh oh!

aferust commented May 10, 2024

Uh oh!

aferust commented May 10, 2024

Uh oh!

aferust commented May 10, 2024

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!

9il commented May 10, 2024

Uh oh!

aferust commented May 10, 2024

Uh oh!

aferust commented May 10, 2024

Uh oh!

jmh530 commented May 10, 2024

Uh oh!

aferust commented May 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aferust May 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

aferust commented May 11, 2024 via email

Uh oh!

aferust commented May 11, 2024

Uh oh!

9il commented May 12, 2024

Uh oh!

aferust commented May 13, 2024

Uh oh!

jmh530 commented May 13, 2024

Uh oh!

9il commented May 14, 2024

Uh oh!

jmh530 commented May 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

9il May 10, 2024 •

edited

Loading

9il commented May 10, 2024 •

edited

Loading

aferust May 10, 2024 •

edited

Loading