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.

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

Unit tests for out argument to processors #1805

Merged
merged 42 commits into from
Aug 22, 2024
Merged

Unit tests for out argument to processors #1805

merged 42 commits into from
Aug 22, 2024

Conversation

hrobarts
Copy link
Contributor

@hrobarts hrobarts commented May 14, 2024

Description

Add tests to check if processors work in place. Check if processors give the same result when:

  • Getting results directly with return
processor.set_input(data)
out = processor.get_output()
  • Getting result by editing in place
processor.set_input(data)
processor.get_output(out=data)
  • Getting result with an empty out array. Check also that this doesn't change the input data and that the processor does not suppress the return value.
input = data.copy()
out=0*(data.copy())
processor.set_input(input)
processor.get_output(out=out)

Added a check_output function in Processor to check the output type and size of an out argument that's passed to the prcessors. Slicer, Binner, Padded and MaskGenerator over-ride the base class to allow different type or size out argument.

Added a _set_up function in Processor to set the _shape_out attribute and other attributes that need to be set from the data.

CentreOfRotationCorrector only changes the data.geometry, not the data, so out_test fails. Instead we test the geometry is the same when used with or without out

Also update the documentation for Processor to reflect that the return value is not suppressed when out is passed

Added a new test to testclass.py to check if two DataContainers are close, by checking

  • they are the same class
  • they have arrays that are all close
  • if they have geometry, the geometries are equal
  • and if strict=True, their data type is the same

Testing you performed

New tests have been added to test_out_in_place

Related issues/links

Should close #1669
Similar to #1657 for processors

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 marked this pull request as ready for review May 15, 2024 09:47
@hrobarts hrobarts self-assigned this May 15, 2024
@hrobarts
Copy link
Contributor Author

Thank you for the review @MargaretDuff :)

Copy link
Member

@MargaretDuff MargaretDuff left a comment

Choose a reason for hiding this comment

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

Thanks for updating that Hannah! I am happy to approve.

Might suggest that we try and add some more comments to the unit tests so that if we implement new processors it is clear how add tests for them?

@hrobarts
Copy link
Contributor Author

Thanks for updating that Hannah! I am happy to approve.

Might suggest that we try and add some more comments to the unit tests so that if we implement new processors it is clear how add tests for them?

Thanks Margaret, I've added a few comments for new processors

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.

Some changes in the tests, but nothing major needed.

CHANGELOG.md Outdated Show resolved Hide resolved
Wrappers/Python/test/test_out_in_place.py Outdated Show resolved Hide resolved
Wrappers/Python/test/test_out_in_place.py Outdated Show resolved Hide resolved
Wrappers/Python/test/test_out_in_place.py Outdated Show resolved Hide resolved
Wrappers/Python/test/test_out_in_place.py Outdated Show resolved Hide resolved
Wrappers/Python/test/test_out_in_place.py Outdated Show resolved Hide resolved
Wrappers/Python/test/test_out_in_place.py Outdated Show resolved Hide resolved
Wrappers/Python/test/test_out_in_place.py Outdated Show resolved Hide resolved
Wrappers/Python/test/test_out_in_place.py Outdated Show resolved Hide resolved
Wrappers/Python/test/test_out_in_place.py Outdated Show resolved Hide resolved
@hrobarts
Copy link
Contributor Author

Thank you for the review Gemma, I think all the comments are resolved now

@hrobarts hrobarts requested a review from gfardell July 25, 2024 11:58
Wrappers/Python/cil/plugins/tigre/FBP.py Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
Wrappers/Python/cil/framework/framework.py Show resolved Hide resolved
Wrappers/Python/test/testclass.py Outdated Show resolved Hide resolved
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.

It looks great. And very good finding the hidden processors in plugins!

@hrobarts hrobarts merged commit 66206d3 into master Aug 22, 2024
8 checks passed
@hrobarts hrobarts deleted the processors_out branch August 22, 2024 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unit tests for processors with an out argument to check they are safe to use in place
3 participants