Skip to content

Fixed sirf.Reg.ImageData.asarray().#1376

Merged
KrisThielemans merged 28 commits intomasterfrom
reg-asarray-fix
Mar 27, 2026
Merged

Fixed sirf.Reg.ImageData.asarray().#1376
KrisThielemans merged 28 commits intomasterfrom
reg-asarray-fix

Conversation

@evgueni-ovtchinnikov
Copy link
Copy Markdown
Contributor

@evgueni-ovtchinnikov evgueni-ovtchinnikov commented Feb 17, 2026

Changes in this pull request

Testing performed

Tested by sirf.Utilities.data_container_algebra_tests.

Related issues

Fixes #1375

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.

@evgueni-ovtchinnikov evgueni-ovtchinnikov changed the title sirf.Reg.ImageData.asarray() fix Fixed sirf.Reg.ImageData.asarray(). Feb 17, 2026
Copy link
Copy Markdown
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.

Need to say something about the Reg fix in CHANGES.md.

As the new functionality actually tested? As I don't think it's correct.

Also, in #1373 I suggested to duplicate tests with as_array() and asarray(), but maybe that isn't necessary.

Final comment, this is currently on top of #1373. Are we abandoning that one? The history here is rather awful, but a squash merge if the Reg fix and the algebra fix seems rather weird. Would then need some rebasing. Alternatively, we squash merge #1373 first, and then merge master here etc. No easy solutions... @casperdcl, we'll need your help I think.

@evgueni-ovtchinnikov
Copy link
Copy Markdown
Contributor Author

@KrisThielemans To clean up the history, I suggest we merge #1373 before #1376.

@KrisThielemans
Copy link
Copy Markdown
Member

ok, this is now a very small PR. Great! Once ready, please use squash-merge.

Question: is the output of the old as_array() and new asarray() (and presumably therefore new as_array(), as you're testing that) the same? (i.e. do they use the same order). I guess so, as you haven't touched Reg.ImageData.as_array(), which is currently

array = numpy.ndarray(dim, dtype=numpy.float32, order='F')

We cannot change that, as otherwise, we'll get into trouble with the registration tools and geometry.

This does explain some choices that Richard made in the geometric info code (which don't work for STIR, but that's another PR)

@evgueni-ovtchinnikov
Copy link
Copy Markdown
Contributor Author

yes, as_array returns the same data as asarray

@KrisThielemans please approve, I cannot merge until you approve

Copy link
Copy Markdown
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.

All fine, except that CHANGES.md was based on an old version, so you lost some lines.

@KrisThielemans KrisThielemans merged commit da6d50c into master Mar 27, 2026
3 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in 2026-03 AIRBI Hackathon Mar 27, 2026
@KrisThielemans KrisThielemans deleted the reg-asarray-fix branch March 27, 2026 17:30
@KrisThielemans KrisThielemans added this to the v3.10.0 milestone Mar 27, 2026
@KrisThielemans KrisThielemans self-assigned this Mar 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Development

Successfully merging this pull request may close these issues.

The return value of sirf.Reg.ImageData.asarray() is wrong.

3 participants