fix: convert em_rates to backend array in Identity.render (#124)#140
fix: convert em_rates to backend array in Identity.render (#124)#140parallelArchitect wants to merge 8 commits into
Conversation
…#124) Identity.render() declared **kwargs instead of an explicit xp parameter, so xp was never in scope to convert em_rates to the active backend. truth is converted via xp in the ground_truth() path; em_rates from filtered_emission_rates() was not, causing a TypeError when multiplying a backend array (cupy) against an unconverted numpy array. Fix mirrors the working solution from the issue reporter's fork: em_rates.copy(data=xp.asarray(...)) preserves the xarray wrapper so .coords access below still works. Verified against both cupy and numpy backends, using synthetic MatsLines data and real COSEM segmentation data (jrc_hela-3, er-mem_pred).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #140 +/- ##
==========================================
+ Coverage 85.48% 85.59% +0.11%
==========================================
Files 46 46
Lines 2963 2965 +2
==========================================
+ Hits 2533 2538 +5
+ Misses 430 427 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
xp as a keyword-only param via **kwargs violated Liskov substitution against _PSFModality.render()'s signature. Matches base class exactly: truth, em_rates, objective_lens, settings, xp as named params. Re-verified GPU (cupy) and CPU (numpy) backends after the signature change.
Covers Identity.render() backend conversion across numpy and cupy (parametrized via the existing np_backend fixture). Scoped narrowly to optical_image() — the boundary of what tlambert03#124 actually touches — to avoid an unrelated, pre-existing failure in the detector simulation step that surfaces when running the full pipeline.
|
The 11 failing CI jobs here are unrelated to this fix — all of them Filed and fixed separately in #141 (adds "zarr3" to the ImageFormat Coverage gap from earlier is also closed — added a regression test |
|
thanks for digging into this too! I'm working on unpinning numpy and solidifying the pinnings for 3.12-3.14, so the dev notes (which are much appreciated!) might not be relevant shortly. Will come back to this tomorrow or the next day either way |
|
Thanks for the context on the numpy unpinning — good to know the dev notes may have a short shelf life. Happy to rebase or update anything once the pinning work settles. Looking forward to your feedback on #140. |
2 participants
Identity.render() declared **kwargs instead of an explicit xp parameter, so xp was never in scope to convert em_rates to the active backend. truth is converted via xp in the ground_truth() path; em_rates from filtered_emission_rates() was not, causing a TypeError when multiplying a backend array (cupy) against an unconverted numpy array.
Fix mirrors the working solution from the issue reporter's fork: em_rates.copy(data=xp.asarray(...)) preserves the xarray wrapper so .coords access below still works. Verified against both cupy and numpy backends, using synthetic MatsLines data and real COSEM segmentation data (jrc_hela-3, er-mem_pred).
Re: the simpler version discussed in the issue thread
(xp.asarray(em_rates.sum(Axis.W).data) without .copy()) — that
returns a raw array with no .coords attribute, which would break
the em_rates.coords[Axis.C] reference a few lines down in the
return statement. That's likely why it wasn't testable as-is.