Skip to content

Conversation

@hrobarts
Copy link
Contributor

@hrobarts hrobarts commented Mar 6, 2025

Description

Check if skimage is installed before tests that require it.

Now skips tests if skimage not installed

Closes #2165

blame GH for this text: https://github.com/orgs/community/discussions/81319

Changes

Testing you performed

Please add any demo scripts to https://github.com/TomographicImaging/CIL-Demos/tree/main/misc

Related issues/links

Checklist

  • I have performed a self-review of my code
  • I have added docstrings in line with the guidance in the developer guide
  • I have updated the relevant documentation
  • I have implemented unit tests that cover any new or modified functionality
  • CHANGELOG.md has been updated with any functionality change
  • Request review from all relevant developers
  • Change pull request label to 'Waiting for review'

Contribution Notes

Please read and adhere to the developer guide and local patterns and conventions.

  • The content of this Pull Request (the Contribution) is intentionally submitted for inclusion in CIL (the Work) under the terms and conditions of the Apache-2.0 License
  • I confirm that the contribution does not violate any intellectual property rights of third parties

@hrobarts hrobarts self-assigned this Mar 6, 2025
@hrobarts hrobarts marked this pull request as ready for review March 7, 2025 15:07
@hrobarts hrobarts changed the title Update installation instructions New environment files for installing tested environments with CIL Mar 17, 2025
@hrobarts hrobarts requested a review from gfardell March 19, 2025 09:24
Copy link
Member

@gfardell gfardell left a comment

Choose a reason for hiding this comment

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

I'll run and check they resolve!

Is this where we want them to live?

@gfardell
Copy link
Member

This might take some thinking about. Considering first the basic cil_env.yml, it installs cil=24.3.0.

Below are my outputs (truncated), running the v24.3.0 tests. It seems we need to add a skimage skip, and I believe we knew the test_bb_converge was having windows issues on release.

I think this means potentially we need a cil_env_dev.yml, cil_demos_dev.yml, cil_tests_dev.yml that create the environment, and run GHA in CI. This will ensure that as we add tests they get correctly run or skipped depending on the environment. This is potentially as simple as toggling ccpi::cil to ccpi/label/dev::cil in the files.

Separately we could have periodic tests to confirm this tagged environment can still be created and pass tests. The tests that need to pass are those from the associated tag.

v24.3.0 tests:

----------------------------------------------------------------------
TEST SYSTEM CONFIGURATION
CIL version:  24.3.0
{'has_astra': False,
 'has_ccpi_regularisation': False,
 'has_cvxpy': False,
 'has_ipp': True,
 'has_numba': True,
 'has_nvidia': True,
 'has_tigre': False,
 'has_tomophantom': False}
----------------------------------------------------------------------



======================================================================
ERROR: test_reconstructors (unittest.loader._FailedTest.test_reconstructors)
----------------------------------------------------------------------
ImportError: Failed to import test module: test_reconstructors
Traceback (most recent call last):
  File "C:\Users\tpc56154\miniforge3\envs\cil\Lib\unittest\loader.py", line 396, in _find_test_path
    module = self._get_module_from_name(name)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\tpc56154\miniforge3\envs\cil\Lib\unittest\loader.py", line 339, in _get_module_from_name
    __import__(name)
  File "C:\Users\tpc56154\GitHub\CIL\Wrappers\Python\test\test_reconstructors.py", line 23, in <module>
    from skimage.transform.radon_transform import _get_fourier_filter as skimage_get_fourier_filter
ModuleNotFoundError: No module named 'skimage'


======================================================================
FAIL: test_bb_converge (test_stepsizes.TestStepSizeBB.test_bb_converge)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Users\tpc56154\GitHub\CIL\Wrappers\Python\test\test_stepsizes.py", line 303, in test_bb_converge
    self.assertNumpyArrayAlmostEqual(alg.x.as_array(), alg_true.x.as_array(), decimal=2)
  File "C:\Users\tpc56154\GitHub\CIL\Wrappers\Python\test\testclass.py", line 60, in assertNumpyArrayAlmostEqual
    np.testing.assert_array_almost_equal(first, second, decimal)
  File "C:\Users\tpc56154\miniforge3\envs\cil\Lib\contextlib.py", line 81, in inner
    return func(*args, **kwds)
           ^^^^^^^^^^^^^^^^^^^
  File "C:\Users\tpc56154\miniforge3\envs\cil\Lib\site-packages\numpy\testing\_private\utils.py", line 1034, in assert_array_almost_equal
    assert_array_compare(compare, x, y, err_msg=err_msg, verbose=verbose,
  File "C:\Users\tpc56154\miniforge3\envs\cil\Lib\contextlib.py", line 81, in inner
    return func(*args, **kwds)
           ^^^^^^^^^^^^^^^^^^^
  File "C:\Users\tpc56154\miniforge3\envs\cil\Lib\site-packages\numpy\testing\_private\utils.py", line 797, in assert_array_compare
    raise AssertionError(msg)
AssertionError:
Arrays are not almost equal to 2 decimals

Mismatched elements: 10 / 10 (100%)
Max absolute difference: 0.04203627
Max relative difference: 0.2293641
 x: array([-0.23, -0.26,  0.51,  0.24,  0.5 , -0.51,  0.2 ,  0.39, -0.61,
        0.6 ], dtype=float32)
 y: array([-0.18, -0.24,  0.54,  0.27,  0.52, -0.48,  0.23,  0.43, -0.58,
        0.62], dtype=float32)

----------------------------------------------------------------------
Ran 950 tests in 84.803s

FAILED (failures=1, errors=1, skipped=251)
(cil)

@gfardell
Copy link
Member

cil_demos environnment & tests on windows.

----------------------------------------------------------------------
TEST SYSTEM CONFIGURATION
CIL version:  24.3.0
{'has_astra': True,
 'has_ccpi_regularisation': True,
 'has_cvxpy': False,
 'has_ipp': True,
 'has_numba': True,
 'has_nvidia': True,
 'has_tigre': True,
 'has_tomophantom': True}
----------------------------------------------------------------------

======================================================================
FAIL: test_bb_converge (test_stepsizes.TestStepSizeBB.test_bb_converge)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Users\tpc56154\GitHub\CIL\Wrappers\Python\test\test_stepsizes.py", line 303, in test_bb_converge
    self.assertNumpyArrayAlmostEqual(alg.x.as_array(), alg_true.x.as_array(), decimal=2)
  File "C:\Users\tpc56154\GitHub\CIL\Wrappers\Python\test\testclass.py", line 60, in assertNumpyArrayAlmostEqual
    np.testing.assert_array_almost_equal(first, second, decimal)
  File "C:\Users\tpc56154\miniforge3\envs\cil_demos\Lib\contextlib.py", line 81, in inner
    return func(*args, **kwds)
           ^^^^^^^^^^^^^^^^^^^
  File "C:\Users\tpc56154\miniforge3\envs\cil_demos\Lib\site-packages\numpy\testing\_private\utils.py", line 1034, in assert_array_almost_equal
    assert_array_compare(compare, x, y, err_msg=err_msg, verbose=verbose,
  File "C:\Users\tpc56154\miniforge3\envs\cil_demos\Lib\contextlib.py", line 81, in inner
    return func(*args, **kwds)
           ^^^^^^^^^^^^^^^^^^^
  File "C:\Users\tpc56154\miniforge3\envs\cil_demos\Lib\site-packages\numpy\testing\_private\utils.py", line 797, in assert_array_compare
    raise AssertionError(msg)
AssertionError:
Arrays are not almost equal to 2 decimals

Mismatched elements: 10 / 10 (100%)
Max absolute difference: 0.04203627
Max relative difference: 0.2293641
 x: array([-0.23, -0.26,  0.51,  0.24,  0.5 , -0.51,  0.2 ,  0.39, -0.61,
        0.6 ], dtype=float32)
 y: array([-0.18, -0.24,  0.54,  0.27,  0.52, -0.48,  0.23,  0.43, -0.58,
        0.62], dtype=float32)

----------------------------------------------------------------------
Ran 981 tests in 151.968s

FAILED (failures=1, skipped=54)

@gfardell
Copy link
Member

cil_test env & windows

----------------------------------------------------------------------
TEST SYSTEM CONFIGURATION
CIL version:  24.3.0
{'has_astra': True,
 'has_ccpi_regularisation': True,
 'has_cvxpy': True,
 'has_ipp': True,
 'has_numba': True,
 'has_nvidia': True,
 'has_tigre': True,
 'has_tomophantom': True}
----------------------------------------------------------------------

======================================================================
FAIL: test_bb_converge (test_stepsizes.TestStepSizeBB.test_bb_converge)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Users\tpc56154\GitHub\CIL\Wrappers\Python\test\test_stepsizes.py", line 303, in test_bb_converge
    self.assertNumpyArrayAlmostEqual(alg.x.as_array(), alg_true.x.as_array(), decimal=2)
  File "C:\Users\tpc56154\GitHub\CIL\Wrappers\Python\test\testclass.py", line 60, in assertNumpyArrayAlmostEqual
    np.testing.assert_array_almost_equal(first, second, decimal)
  File "C:\Users\tpc56154\miniforge3\envs\cil_test\Lib\contextlib.py", line 81, in inner
    return func(*args, **kwds)
           ^^^^^^^^^^^^^^^^^^^
  File "C:\Users\tpc56154\miniforge3\envs\cil_test\Lib\site-packages\numpy\testing\_private\utils.py", line 1034, in assert_array_almost_equal
    assert_array_compare(compare, x, y, err_msg=err_msg, verbose=verbose,
  File "C:\Users\tpc56154\miniforge3\envs\cil_test\Lib\contextlib.py", line 81, in inner
    return func(*args, **kwds)
           ^^^^^^^^^^^^^^^^^^^
  File "C:\Users\tpc56154\miniforge3\envs\cil_test\Lib\site-packages\numpy\testing\_private\utils.py", line 797, in assert_array_compare
    raise AssertionError(msg)
AssertionError:
Arrays are not almost equal to 2 decimals

Mismatched elements: 10 / 10 (100%)
Max absolute difference: 0.04203627
Max relative difference: 0.2293641
 x: array([-0.23, -0.26,  0.51,  0.24,  0.5 , -0.51,  0.2 ,  0.39, -0.61,
        0.6 ], dtype=float32)
 y: array([-0.18, -0.24,  0.54,  0.27,  0.52, -0.48,  0.23,  0.43, -0.58,
        0.62], dtype=float32)

----------------------------------------------------------------------
Ran 981 tests in 149.922s

FAILED (failures=1, skipped=40)

Copy link
Member

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

due to #2118, the recipe/cil_*.yml files can be moved to sections in pyproject.toml's [dependency-groups] table

@casperdcl
Copy link
Member

casperdcl commented Apr 24, 2025

alternatively, shouldn't we add these files to the CIL-Demos repo directly instead of here?

Then CIL's CI could do

pytest .

git clone https://github.com/TomographicImaging/CIL-Demos
sed -ri '/- cil($|\s|[<>=])/d' ./CIL-Demos/environment.yml # remove CIL
conda install --file ./CIL-Demos/environment.yml

pytest --nbval ./CIL-Demos/.../some_notebook.ipynb

@hrobarts hrobarts changed the title New environment files for installing tested environments with CIL Check if environment has skimage before running tests May 27, 2025
@hrobarts
Copy link
Contributor Author

Moved the environment files to a separate repo https://github.com/TomographicImaging/Build-scripts/pull/1]
This PR just has checks for skimage before running tests

@hrobarts hrobarts requested review from casperdcl and gfardell May 27, 2025 07:47
@hrobarts
Copy link
Contributor Author

hrobarts commented Jun 10, 2025

Tested skipping skimage using:

  • Cil_env

    • On linux: Ran 1003 tests in 115.496s OK (skipped=283) - skipped skimage and other stuff
    • On windows: Ran 1003 tests in 169.757s OK (skipped=283) 0 skipped skimage and other stuff
  • Cil_demos_env

    • On linux: Ran 1003 tests in 238.524s OK (skipped=55) - skipped SIRF and CVXpy
    • On windows: Ran 1003 tests in 298.059s OK (skipped=55) - skipped SIRF and CVXpy
  • Cil_test_env

    • On linux: Ran 1003 tests in 229.406s OK (skipped=40) - skipped SIRF
    • On windows:

Copy link
Member

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

I'm not going to approve as this is deeply unpythonic, but I'm also not going to request changes as it's committing the same crimes as the rest of the existing codebase.

I wash my hands off this abomination :)

cough #2172

@hrobarts hrobarts merged commit eae0392 into master Jun 10, 2025
10 checks passed
@hrobarts hrobarts deleted the inst_instructions branch June 10, 2025 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tests fail if skimage not installed

5 participants