ProjectionOperator Bug Fix (Issue #1990) - #2065
Conversation
… 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]>
|
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 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 No issues running tests in local env:
|
…/CIL into projectionop_bug_1990
…raProjectors inherit from ParametrizedTestCase
|
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
left a comment
There was a problem hiding this comment.
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.
…ecutes pip install if list is not empty
|
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. |
…/CIL into projectionop_bug_1990
Signed-off-by: M-A-Demir <[email protected]>
…/CIL into projectionop_bug_1990
Co-authored-by: Hannah Robarts <[email protected]> Signed-off-by: M-A-Demir <[email protected]>
Co-authored-by: Hannah Robarts <[email protected]> Signed-off-by: M-A-Demir <[email protected]>
Co-authored-by: Hannah Robarts <[email protected]> Signed-off-by: M-A-Demir <[email protected]>
hrobarts
left a comment
There was a problem hiding this comment.
Looks good to me! Tests look really good and the new details in the developer guide will be really useful.
gfardell
left a comment
There was a problem hiding this comment.
This is great, thanks for your contribution Mariam!
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!