Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

re-enable some CIL tests depending on SIRF allocate() #846

Merged
merged 3 commits into from
Mar 1, 2024
Merged

re-enable some CIL tests depending on SIRF allocate() #846

merged 3 commits into from
Mar 1, 2024

Conversation

KrisThielemans
Copy link
Member

Now that SyneRBI/SIRF#1212 is fixed, we can reenable relevant tests

Reverts 160d5e5

@KrisThielemans KrisThielemans added this to the v3.6 milestone Nov 22, 2023
@casperdcl
Copy link
Member

casperdcl commented Nov 22, 2023 •
edited
Loading

Based on the failing CI doesn't look like it's fixed yet /CC @evgueni-ovtchinnikov 😅

@KrisThielemans
Copy link
Member Author

Not so clear if there's still a SIRF problem. It'll only be the DEVEL_BUILD=ON jobs that would work at the moment. That creates suitable complications...

Looks like only one of your CI jobs tests with DEVEL_BUILD=ON:

SIRF-SuperBuild/.github/workflows/c-cpp.yml

Lines 58 to 63 in 42ce8cb

- os: ubuntu-latest
compiler: gcc
compiler_version: 11
BUILD_TYPE: "Release"
EXTRA_BUILD_FLAGS: "-DUSE_ITK=ON -DBUILD_CIL=ON -DUSE_SYSTEM_ACE=ON -DUSE_SYSTEM_Armadillo=OFF -DUSE_SYSTEM_Boost=ON -DUSE_SYSTEM_FFTW3=ON -DUSE_SYSTEM_HDF5=ON -DBUILD_siemens_to_ismrmrd=ON -DUSE_SYSTEM_SWIG=ON -DUSE_ROOT:BOOL=ON"
DEVEL_BUILD: "ON"

Let's hope that https://github.com/SyneRBI/SIRF-SuperBuild/actions/runs/6956888225/job/18928662469?pr=846 works therefore, and the docker devel build (but that'll likely get aobrted first if one of the others fails).

In fact, we really should do the SIRF_CIL tests are part if the SIRF CI, but they aren't. I suppose we're not building CIL there. Oh well.

PS: By the way, it's a pain to figure out which job is which. we should do something smarter with naming.

@evgueni-ovtchinnikov
Copy link
Contributor

evgueni-ovtchinnikov commented Nov 22, 2023 •
edited
Loading

I did not expect the use of allocate(None) (looks bizarre to me), quick fix for backward compatibility just pushed.

...to the wrong branch

pushed to master now

@KrisThielemans
Copy link
Member Author

Unrelated failure in DEVEL_BUILD=ON](https://github.com/SyneRBI/SIRF-SuperBuild/actions/runs/6956888225/job/18928662469?pr=846#step:11:1095) and the docker devel-service build: TomographicImaging/CIL#1587

@KrisThielemans
Copy link
Member Author

By the way, no reason we test the docker CIL tests only in the service images I guess

@KrisThielemans KrisThielemans changed the title renable some CIL tests depending on SIRF allocate() re-enable some CIL tests depending on SIRF allocate() Nov 23, 2023
@KrisThielemans
Copy link
Member Author

Possibly needs version_config.cmake update for SIRF_TAG to current version, but not sure if that's what is still the problem.

@KrisThielemans
renable some CIL tests depending on SIRF allocate()
2f94642 
Now that SyneRBI/SIRF#1212 is fixed, we can reenable
relevant tests

Reverts SyneRBI@160d5e5
@KrisThielemans
Copy link
Member Author

just rebased this. After all the recent changes, maybe it'll work...

@KrisThielemans
Copy link
Member Author

Fails with

connection to gadgetron server failed, trying again

We do try and run it

SIRF-SuperBuild/.github/workflows/test_cil.sh

Line 12 in 8ca5067

$SIRF_PATH/../../INSTALL/bin/gadgetron >& ~/gadgetron.log&

it is executed https://github.com/SyneRBI/SIRF-SuperBuild/actions/runs/8092243834/job/22112637500#step:10:41, but we don't see the log

Possibly due to #722

@KrisThielemans
Copy link
Member Author

@casperdcl to edit .github/workflows/test_cil.sh: remove explicit paths and possibly we need to source env_sirf.sh first.

@casperdcl
Copy link
Member

casperdcl commented Feb 29, 2024 •
edited
Loading

https://github.com/TomographicImaging/CIL/blob/v23.1.0/Wrappers/Python/test/test_SIRF.py one test failing:

test_TVdenoisingMR (test_SIRF.TestGradientMR_2D) ...
WARNING:root:C backend is only for arrays of datatype float32 - defaulting to `numpy` backend
...
WARNING:root:C backend is only for arrays of datatype float32 - defaulting to `numpy` backend
FAIL
...
======================================================================
FAIL: test_TVdenoisingMR (test_SIRF.TestGradientMR_2D)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/jovyan/cil_sirf_test/test_SIRF.py", line 259, in test_TVdenoisingMR
    np.testing.assert_array_almost_equal(res1.as_array(), res2.as_array(), decimal=3)
Iterations stopped at 100 with the residual 0.005245 
Iterations stopped at 100 with the residual 0.000000 
Iterations stopped at 100 with the residual 0.000500 
  File "/opt/conda/lib/python3.10/contextlib.py", line 79, in inner
    return func(*args, **kwds)
  File "/opt/conda/lib/python3.10/site-packages/numpy/testing/_private/utils.py", line 1099, in assert_array_almost_equal
    assert_array_compare(compare, x, y, err_msg=err_msg, verbose=verbose,
  File "/opt/conda/lib/python3.10/contextlib.py", line 79, in inner
    return func(*args, **kwds)
  File "/opt/conda/lib/python3.10/site-packages/numpy/testing/_private/utils.py", line 862, in assert_array_compare
    raise AssertionError(msg)
AssertionError: 
Arrays are not almost equal to 3 decimals

Mismatched elements: 74722 / 131072 (57%)
Max absolute difference: 0.14627695
Max relative difference: 0.3511309
 x: array([[[0.198+0.j, 0.195+0.j, 0.191+0.j, ..., 0.203+0.j, 0.202+0.j,
         0.199+0.j],
        [0.198+0.j, 0.196+0.j, 0.193+0.j, ..., 0.204+0.j, 0.202+0.j,...
 y: array([[[0.196+0.j, 0.195+0.j, 0.193+0.j, ..., 0.204+0.j, 0.202+0.j,
         0.201+0.j],
        [0.196+0.j, 0.195+0.j, 0.194+0.j, ..., 0.203+0.j, 0.202+0.j,...

@KrisThielemans
Copy link
Member Author

Well, the good thing is that GradientPET BlockDataContainer tests work!

The failing test is https://github.com/TomographicImaging/CIL/blob/7e0291ea455a4aa037900bf248817a539a7b9d67/Wrappers/Python/test/test_SIRF.py#L249-L258

    def test_TVdenoisingMR(self):
        
        # compare inplace proximal method of TV
        alpha = 0.5
        TV = alpha * TotalVariation(max_iteration=10, warm_start=False)
        res1 = TV.proximal(self.image1, tau=1.0)


        res2 = self.image1*0.
        TV.proximal(self.image1, tau=1.0, out=res2)   
        np.testing.assert_array_almost_equal(res1.as_array(), res2.as_array(), decimal=3)

Checking the error message, it appears that res1 and res2 are almost equal, just not to the required precision. However, that's pretty weird of course. Why would assigning the result be different from storing it in out, unless the iterative process didn't converge yet?

@MargaretDuff @paskino @epapoutsellis is this different due to the warm_start=False in the first call, but not in the second call? If so, it seems a bug in the test.

@MargaretDuff
Copy link

Why would assigning the result be different from storing it in out, unless the iterative process didn't converge yet?

With 10 inner iterations, it may not have converged... But then I don't know where the randomness will have come from as the dual in the proximal calculation is allocated with zeros.

@MargaretDuff @paskino @epapoutsellis is this different due to the warm_start=False in the first call, but not in the second call? If so, it seems a bug in the test.

The warm_start=False call is used when TV is defined not when prox is called so this should be fine.

The TV.proximal is not safe to use in-place at the moment so TV.proximal(x, tau, out=x) is incorrect, but this should be ok here because you use TV.proximal(x, tau, out=x*0)?

@casperdcl
Copy link
Member

The tests were fixed in CIL but not yet released. Pushed a temp fix for now, to be reverted after TomographicImaging/CIL#1732.

I suggest we merge this & open a follow-up issue to track TomographicImaging/CIL#1732.

@KrisThielemans KrisThielemans merged commit 6cc8a2c into SyneRBI:master Mar 1, 2024
8 checks passed
@KrisThielemans KrisThielemans deleted the reenable_some_CIL_tests branch March 1, 2024 17:43
@casperdcl casperdcl mentioned this pull request Mar 8, 2024
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@casperdcl casperdcl Awaiting requested review from casperdcl

@paskino paskino Awaiting requested review from paskino

Assignees

@KrisThielemans KrisThielemans

@casperdcl casperdcl

Labels
None yet
Projects
None yet
Milestone
v3.6
Development

Successfully merging this pull request may close these issues.
4 participants
@KrisThielemans @casperdcl @evgueni-ovtchinnikov @MargaretDuff