Skip to content

ProjectionOperator Bug Fix (Issue #1990) - #2065

Merged
M-A-Demir merged 24 commits into
masterfrom
projectionop_bug_1990
Mar 12, 2025
Merged

ProjectionOperator Bug Fix (Issue #1990)#2065
M-A-Demir merged 24 commits into
masterfrom
projectionop_bug_1990

Conversation

@M-A-Demir

@M-A-Demir M-A-Demir commented Feb 6, 2025

Copy link
Copy Markdown
Contributor

Description

Update in response to issue #1990: ProjectionOperator device input is not case-sensitive. Added case sensitivity and unit tests for ProjectionOperator inputs. Updated environment .yml files and env creation script to include new unittest-parametrize library used in tests. Updated developer guide to include example of parametrized test usage. Added name to NOTICE.txt.

Issue: #1990

Example Usage

ProjectionOperator(ig, ag, device="GPU")
device name passed is now case insensitive

❤️ Thanks for your contribution!

M-A-Demir and others added 2 commits February 6, 2025 12:41
… not case-sensitive. Added case sensitivity and unit tests for ProjectionOperator inputs. Updated environment .yml files and env creation script to include new unittest-parametrize library used in tests. Updated developer guide to include example of parametrized test usage. Added name to NOTICE.txt.
formatting

Signed-off-by: M-A-Demir <[email protected]>
@M-A-Demir

M-A-Demir commented Feb 6, 2025

Copy link
Copy Markdown
Contributor Author

PS: In CIL/scripts/create_local_env_for_cil_development.sh, I couldn't install the pip module (unittest-parametrize) directly with the 'conda create' command , so I have changed it to use the requirements_test.yml file if $test_deps != 0.

I tried an alternative, which is to have a separate list of pip dependencies, use conda create, then activate the conda environment and perform pip install pip_dependencies, but ran into an error saying to run conda init before I can do the installations.

in GitHub Actions, Tests using unittest-parametrize produce a TypeError: TestAstraProjectors.test_ProjectionOperator_2Ddata() missing 3 required positional arguments: 'device', 'no_error_raised', and 'err_type'

Thought this may be due to an ASTRA dependency, so added @unittest.skipUnless(has_astra and has_nvidia, "Requires ASTRA GPU") to test_ProjectionOperator_3Ddata, and @unittest.skipUnless(has_astra, "Requires ASTRA") to test_ProjectionOperator_2Ddata - Only the 3D test skips, 2D test does not skip and produces same TypeError.

No issues running tests in local env:

  • python 3.10.15
  • numpy 1.24.4
  • unittest-parametrize 1.6.0

@M-A-Demir M-A-Demir linked an issue Feb 6, 2025 that may be closed by this pull request
@M-A-Demir
M-A-Demir requested a review from hrobarts February 6, 2025 13:20
@M-A-Demir
M-A-Demir requested a review from gfardell February 6, 2025 13:20
@M-A-Demir
M-A-Demir marked this pull request as draft February 6, 2025 13:20
@M-A-Demir
M-A-Demir marked this pull request as ready for review February 6, 2025 13:20
@M-A-Demir M-A-Demir self-assigned this Feb 14, 2025
@M-A-Demir

Copy link
Copy Markdown
Contributor Author

UPDATE: Fixed the TypeError issue on test - the version I pushed did not have the TestAstraProjectors class inherit from unittest_parametrize.ParametrizedTestCase, which is required to run the parametrized tests

@gfardell gfardell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's a really nice addition thanks @M-A-Demir!

In addition to the small comments, could you test this on windows. @hrobarts should be able to help you out.

Comment thread Wrappers/Python/test/test_PluginsAstra_Projectors.py Outdated
Comment thread Wrappers/Python/test/test_PluginsAstra_Projectors.py Outdated
Comment thread scripts/create_local_env_for_cil_development.sh
@M-A-Demir
M-A-Demir requested a review from paskino February 14, 2025 15:13
@hrobarts

Copy link
Copy Markdown
Contributor

Hi @M-A-Demir, this is looking good to me. Could you summarise these updates in the changelog. I think you also need to add the new dependency to the test section in recipe/meta.yaml so GHA can find it for the conda tests. You might need to investigate how to install via pip.

Comment thread Wrappers/Python/test/test_PluginsAstra_Projectors.py
Comment thread Wrappers/Python/test/test_PluginsAstra_Projectors.py
Comment thread Wrappers/Python/test/test_PluginsAstra_Projectors.py
@M-A-Demir
M-A-Demir requested review from gfardell and hrobarts March 7, 2025 11:08

@hrobarts hrobarts left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me! Tests look really good and the new details in the developer guide will be really useful.

@gfardell gfardell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is great, thanks for your contribution Mariam!

@M-A-Demir
M-A-Demir merged commit b5ee284 into master Mar 12, 2025
@M-A-Demir
M-A-Demir deleted the projectionop_bug_1990 branch March 12, 2025 14:10
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.

CGLS/ProjectionOperator Stack Trace Doesn't Point To Correct Issue

3 participants