Skip to content

Refactor .apply_to() in electronic effects - #1006

Open
teutoburg wants to merge 2 commits into
mainfrom
fh/callable
Open

Refactor .apply_to() in electronic effects#1006
teutoburg wants to merge 2 commits into
mainfrom
fh/callable

Conversation

@teutoburg

Copy link
Copy Markdown
Contributor

See #1002 for details and explanation.

  • Add Detector.data property setter to eleminate external access of Detector._hdu attribute.
  • Turn various effect attributes which need currsys/meta resolving into properties. This allows to encapsulate the resolving and make calls to the attributes much cleaner. If we ever make effects dataclasses with explicit attribute fields rather than kwargs, this will make that process much smoother, because accessing a dataclass field looks identical to accessing a property, we just remove the property definitions then.
  • Anything that the .__call__() needs, which is taken from the obj (in this case usually Detector), is resolved before and passed to .__call__() as an argument. That means the latter doesn't need to know about obj, which is cleaner and matches the PSF case.
  • I think there are now a few new np.random.default_rng cases. This was taken from a branch I started several months ago and forgot about in the much more recent discussion about those. Might not be final.

This isn't completely exhaustive for all the effects here, but I tried to cover the "standard" ones...

See #1002 for details and explanation.

- Add `Detector.data` property setter to eleminate external access of
  `Detector._hdu` attribute.
- Turn various effect attributes which need currsys/meta resolving into
  properties. This allows to encapsulate the resolving and make calls to
  the attributes much cleaner. **If we ever make effects dataclasses with
  explicit attribute fields rather than kwargs, this will make that process
  much smoother, because accessing a dataclass field looks identical to
  accessing a property, we just remove the property definitions then**.
- Anything that the `.__call__()` needs, which is taken from the `obj`
  (in this case usually `Detector`), is resolved before and passed to
  `.__call__()` as an argument. That means the latter doesn't need to know
  about `obj`, which is cleaner and matches the PSF case.
- I think there are now a few new `np.random.default_rng` cases. This was
  taken from a branch I started several months ago and forgot about in the
  much more recent discussion about those. Might not be final.

This isn't completely exhaustive for all the effects here, but I tried to
cover the "standard" ones...
@teutoburg teutoburg self-assigned this Aug 31, 2026
@teutoburg teutoburg moved this to 👀 Awaiting Review in ScopeSim-development Aug 31, 2026
@teutoburg teutoburg added refactor Implementation improvement effects Related to a ScopeSim effect labels Aug 31, 2026
@codecov

codecov Bot commented Aug 31, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.49198% with 29 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.47%. Comparing base (ce14dad) to head (779f146).

Files with missing lines Patch % Lines
scopesim/effects/electronic/noise.py 84.53% 15 Missing ⚠️
scopesim/effects/electronic/pixels.py 79.54% 9 Missing ⚠️
scopesim/effects/electronic/electrons.py 89.18% 4 Missing ⚠️
scopesim/effects/rotation.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1006      +/-   ##
==========================================
+ Coverage   76.30%   76.47%   +0.17%     
==========================================
  Files          69       69              
  Lines        9057     9111      +54     
==========================================
+ Hits         6911     6968      +57     
+ Misses       2146     2143       -3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@teutoburg

Copy link
Copy Markdown
Contributor Author

Additional note: This reverts some of the "in-place modifications" of Detector.data, which we previously thought were a good idea, because they have performance and memory advantages. But looking at JAX, in-place modifications are one of the explicit limitations, so for future compatibility I think it's ultimately better to not do that. Also saves us from potential dtype-mismatch issues we had previously on some of these cases...

@teutoburg
teutoburg marked this pull request as ready for review August 31, 2026 21:30
@teutoburg
teutoburg requested a review from a team August 31, 2026 21:30
Somehow forgotten in the previous commit...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effects Related to a ScopeSim effect refactor Implementation improvement

Projects

Status: 👀 Awaiting Review

Development

Successfully merging this pull request may close these issues.

1 participant