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

Zoom image from template method for ImageData #1285

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

samdporter
Copy link
Contributor

@samdporter samdporter commented Aug 21, 2024

Changes in this pull request

Added method for ImageData to zoom image using geometrical information from a template image

Testing performed

  • Compared ImageData zoomed using zoom_image_from_template() and zoom_image() with manually calculated arguments (although dimensions will be different).
  • Compared with zoom_image using stir python

Related issues

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have added docstrings/doxygen in line with the guidance in the developer guide
  • I have implemented unit tests that cover any new or modified functionality
  • The code builds and runs on my machine
  • CHANGES.md has been updated with any functionality change

Contribution Notes

Please read and adhere to the contribution guidelines.

Please tick the following:

  • The content of this Pull Request (the Contribution) is intentionally submitted for inclusion in SIRF (the Work) under the terms and conditions of the Apache-2.0 License.

Copy link
Member

@KrisThielemans KrisThielemans left a comment

Choose a reason for hiding this comment

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

some comments, but in addition,

  • there are some weird/unnecessary white-space differences. Please fix/ remove (see the diff on github). Some of this might be caused by editor conventions with tabs vs spaces, but even your own code doesn't like sensible in places.

  • add a line to CHANGES.md

Comment on lines +1365 to +1369
void zoom_image(const STIRImageData& original_image,
const stir::ZoomOptions zoom_options = stir::ZoomOptions::preserve_sum);

void zoom_image(const STIRImageData& original_image,
const char* const zoom_options_str = "preserve_sum");
Copy link
Member

Choose a reason for hiding this comment

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

naming of parameter would have to be template_image. Needs some doxygen as well. Finally, I'd prefer these to be called exactly the same as the Python version. e.g. zoom_image_as_template

if not isinstance(template, ImageData):
raise error('zoom_image_from_template: template should be ImageData')

### becaus of a bug somewherre
Copy link
Member

Choose a reason for hiding this comment

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

misspellings and indentation problems.

@@ -681,6 +681,24 @@ def zoom_image(self, zooms=(1., 1., 1.), offsets_in_mm=(0., 0., 0.),
np_offsets_in_mm.ctypes.data, np_size.ctypes.data, scaling))

return zoomed_im

def zoom_image_from_template(self, template, scaling='preserve_sum'):
Copy link
Member

Choose a reason for hiding this comment

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

what about zoom_image_as_template?

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.

2 participants