Re: [SyneRBI/SIRF] Add numerical test for Hessian diagonal (PR #1385)

Kris Thielemans

unread,

Mar 24, 2026, 5:00:38 AM (14 days ago) Mar 24

to SyneRBI/SIRF, Subscribed

@KrisThielemans requested changes on this pull request.

In src/xSTIR/pSTIR/tests/tests_qp_lc_rdp.py:

> @@ -61,6 +62,41 @@ def Hessian_test(test, prior, x, eps=1e-3):
     test.check_if_less(q, .01*eps)
 
 
+def Hessian_diagonal_test(test, prior, x, eps=1e-4):
+    """Checks compute_Hessian_diagonal against numerical finite differences.
+
+    For a subset of interior voxels i, checks:
+        H_diag[i] ≈ (grad_i(x + eps*e_i) - grad_i(x)) / eps
+    """
+    H_diag = prior.compute_Hessian_diagonal(x)
+    gx = prior.gradient(x.clone())

why the clone? gradient doesn't modify the argument

In src/xSTIR/pSTIR/STIR.py:

> @@ -2623,6 +2623,7 @@ def compute_Hessian_diagonal(self, image):
         """Computes the diagonal of Hessian"""

let's change this to

⬇️ Suggested change

-        """Computes the diagonal of Hessian"""
+        """Returns the diagonal of Hessian"""

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.

Matteo Colombo

unread,

Mar 24, 2026, 5:26:19 AM (14 days ago) Mar 24

to SyneRBI/SIRF, Push

@ColomboMatte0 pushed 1 commit.

55c3728 remove unnecessary subprocess import to fix Codacy check

—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.

Kris Thielemans

unread,

Mar 24, 2026, 5:43:49 AM (14 days ago) Mar 24

to SyneRBI/SIRF, Subscribed

KrisThielemans left a comment (SyneRBI/SIRF#1385)

Why did you remove subprocess again? It looked like a good idea to me. (I can't seem to find the Codacy complaint)

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.

Matteo Colombo

unread,

Mar 24, 2026, 5:51:10 AM (14 days ago) Mar 24

to SyneRBI/SIRF, Subscribed

ColomboMatte0 left a comment (SyneRBI/SIRF#1385)

It was missing before but i cannot understand why the cudacy check complains about it.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Matteo Colombo

unread,

Mar 24, 2026, 5:53:48 AM (14 days ago) Mar 24

to SyneRBI/SIRF, Push

@ColomboMatte0 pushed 1 commit.

8eb00d0 adding back subprocess and fix bare except

—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.

Kris Thielemans

unread,

Mar 24, 2026, 5:53:57 AM (14 days ago) Mar 24

to SyneRBI/SIRF, Subscribed

KrisThielemans left a comment (SyneRBI/SIRF#1385)

let's ignore Codacy and use your human intelligence :-)

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.

Matteo Colombo

unread,

Mar 24, 2026, 5:56:05 AM (14 days ago) Mar 24

to SyneRBI/SIRF, Subscribed

ColomboMatte0 left a comment (SyneRBI/SIRF#1385)

ok i added it back.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Kris Thielemans

unread,

Mar 24, 2026, 5:56:36 AM (14 days ago) Mar 24

to SyneRBI/SIRF, Subscribed

KrisThielemans left a comment (SyneRBI/SIRF#1385)

oh, it just doesn't like subprocess at all. I presume we could disable this check, but life is short.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Kris Thielemans

unread,

Mar 24, 2026, 5:56:48 AM (14 days ago) Mar 24

to SyneRBI/SIRF, Subscribed

@KrisThielemans approved this pull request.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Kris Thielemans

unread,

Mar 24, 2026, 5:57:47 AM (14 days ago) Mar 24

to SyneRBI/SIRF, Subscribed

KrisThielemans left a comment (SyneRBI/SIRF#1385)