Skip to content

SIRF #1357#1378

Closed
Dimitra-Kyriakopoulou wants to merge 3 commits intoSyneRBI:masterfrom
Dimitra-Kyriakopoulou:Issue_1357
Closed

SIRF #1357#1378
Dimitra-Kyriakopoulou wants to merge 3 commits intoSyneRBI:masterfrom
Dimitra-Kyriakopoulou:Issue_1357

Conversation

@Dimitra-Kyriakopoulou
Copy link
Copy Markdown

@Dimitra-Kyriakopoulou Dimitra-Kyriakopoulou commented Mar 11, 2026

Changes in this pull request

-- Fixed #1357 by applying the same direction already discussed upstream, not a new design.

This patch follows the __array_priority__ / reverse-operator approach that later appears in #1373. #1358 is
already merged.

-- Changes:

  • add __array_priority__ = 10 to ArrayContainer
  • add __radd__, __rsub__, and __rtruediv__ to DataContainer
  • add regression coverage for the reported left-hand cases in src/common/Utilities.py

-- Checked the reported cases: ay * x, 1 + x, ax + x, ax - x, ay / x, 2 / x
These now return SIRF containers instead of numpy.ndarray or TypeError.

Testing performed

  • reran the explicit #1357 repro on the patched branch
  • ran src/xSTIR/pSTIR/tests/tests_data_container_algebra.py -v

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:

  • [x ] 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.

@KrisThielemans
Copy link
Copy Markdown
Member

Thanks @Dimitra-Kyriakopoulou. can you clarify? This is lifting the _array_priority_ to top-level in the SIRF hierarchy?

@Dimitra-Kyriakopoulou
Copy link
Copy Markdown
Author

Dimitra-Kyriakopoulou commented Mar 11, 2026 via email

@KrisThielemans
Copy link
Copy Markdown
Member

ok. @evgueni-ovtchinnikov will review this. thanks!

@evgueni-ovtchinnikov
Copy link
Copy Markdown
Contributor

so far as I can see, this PR just duplicates #1373 - I suggest we rather merge #1373, and if my understanding is correct just drop this one

@Dimitra-Kyriakopoulou
Copy link
Copy Markdown
Author

Dimitra-Kyriakopoulou commented Mar 12, 2026

Dear Professor @evgueni-ovtchinnikov and Professor @KrisThielemans,
I just checked this and Professor Ovtchinnikov is absolutely right. I had myself explicitly referenced #1373, because I understood it as the upstream direction for fixing #1357 rather than as an already complete implementation of it. Because it was still open, and under the time pressure, I did not realise that #1373 already contained the same functional changes, and in fact, as I now see, also more follow-up work (extra result-type checks and docs/changelog updates). I am deeply sorry for taking your time on this, and I wholeheartedly thank Professor Ovtchinnikov for pointing it out and saving me from unintentionally duplicating existing work -in fact in a way that could have come close to indirect plagiarism.

I AM DEEPLY SORRY,
Dimitra

@KrisThielemans
Copy link
Copy Markdown
Member

no problem. Closing this.

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.

backwards incompatibility: algebraic operations with numpy arrays/scalars on the "left" return a numpy array

3 participants