[feat] added peak simulation#3394
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request adds spectrograph-based spectral peak simulation to the simulated camera driver. It introduces a new public utility function Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/odemis/driver/simcam.py`:
- Around line 438-441: The debug logging uses ltrb which is undefined in the
mirror-simulation branch and can raise UnboundLocalError; update the block
around bin_x and peak_center_binned so you only compute and log
peak_center_binned when ltrb is defined (non-mirror path), and for the mirror +
spectrograph case log an alternative value or skip using ltrb (e.g., compute
peak position relative to ROI using the mirror-specific coordinates or guard
with an if checking for the mirror mode or existence of ltrb); modify the code
that sets bin_x and peak_center_binned and the logging statement (refer to
variables/functions x0_px, binning, ltrb, peak_center_binned and the
mirror/spectrograph conditional) so no access to ltrb occurs when mirror
simulation is active.
- Around line 183-186: The current validation for spectrograph in the try block
is inverted and can evaluate spectrograph.axes before confirming it exists;
change the predicate so you first check hasattr(spectrograph, "axes"), then
ensure spectrograph.axes is a dict, and then ensure "goffset" is a key — e.g.
replace the negated compound expression with a positive chain (either use if not
(hasattr(...) and isinstance(... ) and "goffset" in ...): raise ValueError(...)
or use if (not hasattr(...)) or (not isinstance(...)) or ("goffset" not in ...):
raise ValueError(...)) so that spectrograph.axes is only accessed after
confirming it exists.
In `@src/odemis/driver/test/andorshrk_test.py`:
- Line 592: Add a return type annotation -> None to the test_goffset function
and insert a short reStructuredText docstring immediately under the def line
(e.g., a one-line summary and optional brief description) to match repository
style; update the test_goffset function signature and add the docstring inside
the function body so tools and linters pick up the type hint and the
reST-formatted documentation.
- Around line 596-627: The finally block currently only restores goffset causing
test-order coupling; capture the original grating before the try (orig_grating =
sp.position.value["grating"]) and in the finally restore both axes by calling
sp.moveAbsSync with both {"grating": orig_grating, "goffset": orig_pos} (or
restore grating first then goffset) so the spectrograph state (grating and
goffset) is fully restored after the test.
In `@src/odemis/driver/test/simcam_test.py`:
- Around line 518-523: The polling loop may exit without receiving a newer
frame; after the loop that sets image_new by calling self.camera.data.get() and
checks image_new.metadata[model.MD_ACQ_DATE] > date_0, add an explicit assertion
that that condition is true (e.g., self.assertTrue(...) or raise AssertionError)
to abort the test if no fresh frame was received before proceeding to peak
comparisons; reference the variables/methods image_new, self.camera.data.get(),
and model.MD_ACQ_DATE to locate and update the test.
- Around line 460-507: Add reStructuredText docstrings and Python type hints to
the test class and its methods: document TestSimCamSpectrograph (class-level
docstring), MockSpectrograph.__init__ (docstring and annotate parameters/return
as -> None), setUpClass and tearDownClass (annotate cls: Type[...] -> None and
add docstrings), setUp (self -> None + docstring), and _find_peak (annotate
parameter image: numpy.ndarray -> Tuple[float, float, float] and add a docstring
describing behavior/returns). Keep docstrings short, follow reST style (no type
info inside docstrings), and add imports/typing names only if needed to satisfy
the annotations used in these method signatures.
- Around line 499-501: The empty-mask branch in _find_peak incorrectly calls
float() with three args; change the return to construct the 3-tuple with the
first element converted to float and the other two values returned as-is (i.e.,
return float(profile.argmax()), p_max, profile.mean()) so the function returns
(position_as_float, p_max, mean) instead of invoking float with multiple
arguments.
In `@src/odemis/util/synthetic.py`:
- Around line 146-151: Validate and guard the width and dtype before computing
intensity: in the block that computes intensity (variables intensity, width, x0,
amplitude) check that width > 0 and raise a ValueError (or substitute a small
epsilon) to avoid division by zero, and determine whether dtype is integer or
floating via dtype.kind (or numpy.issubdtype) before clipping — use numpy.iinfo
for integer dtypes and numpy.finfo for float dtypes when computing min/max for
numpy.clip and then cast to peak_1d; this ensures
intensity_clipped.astype(dtype) will not fail for float dtypes.
- Around line 131-157: Add type hints to simulate_peak and an reStructuredText
docstring: annotate parameters (e.g., amplitude: float, x0: float, width: float,
shape: Union[int, Sequence[int]], shape may be Tuple[int, ...], shape handling
preserved, image: Optional[numpy.ndarray] = None, dtype: numpy.dtype =
numpy.uint16) and the return type as numpy.ndarray; import Optional and
Sequence/Union from typing as needed. Add a short reST docstring (no types in
it) describing the function purpose and each parameter with :param name: and the
return value with :returns:, and note behavior for 1D vs 2D and when image is
provided; keep existing implementation unchanged and ensure the docstring is
placed immediately below the def simulate_peak(...) line. Ensure imports are
added only if missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 55cc98f5-9cf4-46a8-8b44-6ade0cd5cb72
📒 Files selected for processing (5)
install/linux/usr/share/odemis/sim/sparc2-focus-test.odm.yamlsrc/odemis/driver/simcam.pysrc/odemis/driver/test/andorshrk_test.pysrc/odemis/driver/test/simcam_test.pysrc/odemis/util/synthetic.py
Comment on lines
+518
to
+523
| image_new = None | ||
| for _ in range(10): | ||
| image_new = self.camera.data.get() | ||
| if image_new.metadata[model.MD_ACQ_DATE] > date_0: | ||
| break | ||
| time.sleep(0.05) |
There was a problem hiding this comment.
Assert that a fresh frame was actually received before comparing peaks.
If no newer frame arrives in the polling loop, the test still proceeds and can produce misleading results.
🧪 Proposed hardening
image_new = None
for _ in range(10):
image_new = self.camera.data.get()
if image_new.metadata[model.MD_ACQ_DATE] > date_0:
break
time.sleep(0.05)
+
+ self.assertIsNotNone(image_new)
+ self.assertGreater(
+ image_new.metadata[model.MD_ACQ_DATE],
+ date_0,
+ "Timed out waiting for a fresh frame after goffset change",
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/odemis/driver/test/simcam_test.py` around lines 518 - 523, The polling
loop may exit without receiving a newer frame; after the loop that sets
image_new by calling self.camera.data.get() and checks
image_new.metadata[model.MD_ACQ_DATE] > date_0, add an explicit assertion that
that condition is true (e.g., self.assertTrue(...) or raise AssertionError) to
abort the test if no fresh frame was received before proceeding to peak
comparisons; reference the variables/methods image_new, self.camera.data.get(),
and model.MD_ACQ_DATE to locate and update the test.
pieleric
requested changes
Mar 6, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/odemis/driver/simcam.py (1)
434-457: Verify spectrograph branch independence aligns with maintainer guidance.Per learnings, the spectrograph branch should be independent and not rely on the mirror/standard image simulation. However, the current structure runs the spectrograph branch after either the mirror or standard branch has already computed
sim_img, meaning computation is wasted when spectrograph is enabled.The branch should be restructured as the first condition (
if self._spectrograph:) to avoid running the mirror/standard simulation entirely when spectrograph simulation is active.♻️ Proposed restructuring sketch
def _simulate(self): binning = self.binning.value res = self.resolution.value trans = self.translation.value center = self._img_res[0] / 2, self._img_res[1] / 2 pixel_size = self._metadata.get(model.MD_PIXEL_SIZE, self.pixelSize.value) # Extra translation to simulate stage movement pos = self._metadata.get(model.MD_POS, (0, 0)) pxs = [p / b for p, b in zip(pixel_size, self.binning.value)] stage_shift = pos[0] / pxs[0], -pos[1] / pxs[1] # Y goes opposite - if self._mirror: + if self._spectrograph: + # Independent branch: compute ROI x-start without relying on mirror/standard paths + current_offset = self._spectrograph.position.value["goffset"] + ccd_center_x = self._img_res[0] / 2.0 + x0_px = ccd_center_x + current_offset * GOFFSET_TO_PIXEL + roi_x0 = center[0] + trans[0] + stage_shift[0] - (res[0] / 2) * binning[0] + + bin_x = binning[0] + peak_center_binned = (x0_px - roi_x0) / bin_x + width_binned = PEAK_WIDTH / bin_x + + min_val = self._img.min() # Consider caching at init + sim_img = numpy.full((res[1], res[0]), min_val, dtype=self._img.dtype) + peak = simulate_peak(amplitude=20000, x0=peak_center_binned, width=width_binned, + shape=sim_img.shape, dtype=sim_img.dtype) + sim_img += peak + + elif self._mirror: # ... existing mirror branch ... - else: + else: # ... existing standard branch ... - - # spectrograph peak simulation - if self._spectrograph: - # ... move this logic to the first branch above ...Based on learnings: "the spectrograph branch should be an independent
ifbranch (not run after mirror or standard), computing its ROI x-start independently without relying onltrb."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/driver/simcam.py` around lines 434 - 457, The spectrograph simulation currently runs after the mirror/standard image code and reuses sim_img, causing wasted computation and coupling; refactor so the spectrograph branch is evaluated first (if self._spectrograph:) and returns/sets sim_img independently without executing the mirror/standard branches. In practice move the entire block that computes current_offset, x0_px, roi_left, peak_center_binned, width_binned, calls simulate_peak and assigns sim_img (including setting sim_img[...] = min_val and adding peak) to run before any mirror/standard simulation, ensure it computes ROI start from its own calculations rather than using ltrb, and keep symbols like self._spectrograph, simulate_peak, sim_img, roi_left, peak_center_binned unchanged so callers locate and test the isolated branch easily.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/odemis/driver/simcam.py`:
- Around line 445-447: Replace the malformed call to logging.infologging.info
with a proper debug-level log: call logging.debug instead of logging.info and
pass the message and interpolation arguments correctly (the line referencing
logging.infologging.info and variables x0_px, roi_left, peak_center_binned
should be fixed so it reads a single logging.debug(...) call that formats those
variables). Ensure you remove the duplicate "infologging" token and use
logging.debug for the debug output.
In `@src/odemis/util/synthetic.py`:
- Around line 148-150: Remove the unused variable assignment and its comment:
delete the line that sets resolution_y = shape[0] if len(shape) > 1 else 1 (and
the preceding comment about resolution.y / 1D shape) in
src/odemis/util/synthetic.py so there are no unused locals named resolution_y;
keep any needed logic that computes the actually used resolution(s) intact.
---
Nitpick comments:
In `@src/odemis/driver/simcam.py`:
- Around line 434-457: The spectrograph simulation currently runs after the
mirror/standard image code and reuses sim_img, causing wasted computation and
coupling; refactor so the spectrograph branch is evaluated first (if
self._spectrograph:) and returns/sets sim_img independently without executing
the mirror/standard branches. In practice move the entire block that computes
current_offset, x0_px, roi_left, peak_center_binned, width_binned, calls
simulate_peak and assigns sim_img (including setting sim_img[...] = min_val and
adding peak) to run before any mirror/standard simulation, ensure it computes
ROI start from its own calculations rather than using ltrb, and keep symbols
like self._spectrograph, simulate_peak, sim_img, roi_left, peak_center_binned
unchanged so callers locate and test the isolated branch easily.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 11867f64-d802-4035-b59a-72cb0602489f
📒 Files selected for processing (2)
src/odemis/driver/simcam.pysrc/odemis/util/synthetic.py
yxkdejong
force-pushed
the
add-spectrum-simulation
branch
from
3ed6c61 to
b68f4a7
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/odemis/util/synthetic.py (2)
131-141: 🛠️ Refactor suggestion | 🟠 MajorAdd type hints for all function parameters.
The function has a return type hint but is missing type hints for all parameters (
amplitude,x0,width,shape,dtype). As per coding guidelines, Python code must include type hints for function parameters and return types.🛠️ Proposed fix
-def simulate_peak(amplitude, x0, width, shape, dtype=numpy.uint16) -> numpy.ndarray: +def simulate_peak( + amplitude: float, + x0: float, + width: float, + shape: Union[int, Tuple[int, ...]], + dtype=numpy.uint16, +) -> numpy.ndarray:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/util/synthetic.py` around lines 131 - 141, Add explicit type hints to the simulate_peak signature: annotate amplitude: float, x0: float, width: float, shape: Union[int, Tuple[int, int]], and dtype: Union[numpy.dtype, Type[numpy.generic]] = numpy.uint16 (or use numpy.typing.NDArray/ numpy.dtype types if your codebase prefers), while keeping the existing -> numpy.ndarray return; update imports to include typing.Union, typing.Tuple (and Type if used) or use numpy.typing aliases so the function signature for simulate_peak(amplitude, x0, width, shape, dtype=...) is fully typed.
147-154:⚠️ Potential issue | 🟡 MinorGuard against
width <= 0and handle float dtypes.Two issues remain unaddressed:
- Line 149: Division by
widthwithout validation will raiseZeroDivisionErroror produceinf/nanwhenwidth <= 0.- Line 152:
numpy.iinfo(dtype)raisesValueErrorfor float dtypes.🛠️ Proposed fix
# Generate the 1D Gaussian peak across the x-dimension + if width <= 0: + raise ValueError("width must be positive") + x = numpy.arange(shape[-1]) - intensity = amplitude*(numpy.exp(-0.5 * ((x - x0) / width)**2)) + intensity = amplitude * (numpy.exp(-0.5 * ((x - x0) / width)**2)) # Clip based on the actual dtype provided - info = numpy.iinfo(dtype) + dtype = numpy.dtype(dtype) + if numpy.issubdtype(dtype, numpy.integer): + info = numpy.iinfo(dtype) + else: + info = numpy.finfo(dtype) intensity_clipped = numpy.clip(intensity, info.min, info.max)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/util/synthetic.py` around lines 147 - 154, The intensity computation in synthetic.py can divide by zero when width <= 0 and incorrectly calls numpy.iinfo for float dtypes; update the block around x, width, intensity, numpy.iinfo, intensity_clipped and peak_1d to first validate width (e.g., if width <= 0 raise a clear ValueError or set width = max(width, tiny_eps) depending on desired behavior) and then choose bounds using numpy.iinfo(dtype) for integer dtypes and numpy.finfo(dtype) for float dtypes (use numpy.issubdtype(dtype, numpy.integer) / numpy.issubdtype(dtype, numpy.floating) to branch), applying clipping only with the appropriate min/max before astype to produce peak_1d.src/odemis/driver/simcam.py (1)
445-447:⚠️ Potential issue | 🟡 MinorUse
logging.debugand fix the format string variable name.Two issues:
- Debug-level output should use
logging.debug(), notlogging.info()with a "DEBUG:" prefix.- The format string references
ltrb0but the actual variable isroi_left.🐛 Proposed fix
- logging.info("DEBUG: x0_px=%s, ltrb0=%s, result=%s", - x0_px, roi_left, peak_center_binned - ) + logging.debug("x0_px=%s, roi_left=%s, peak_center_binned=%s", + x0_px, roi_left, peak_center_binned)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/driver/simcam.py` around lines 445 - 447, Change the diagnostic log call in simcam.py from logging.info to logging.debug and remove the literal "DEBUG:" prefix; also correct the format-string variable name to match the actual variable (use roi_left, not ltrb0) so the call becomes a logging.debug(...) with the format and arguments (x0_px, roi_left, peak_center_binned) in the same order; locate and update the log statement near the code that computes x0_px, roi_left and peak_center_binned (inside the related function handling peak_center_binned).
🧹 Nitpick comments (1)
src/odemis/driver/simcam.py (1)
434-457: Spectrograph branch runs after mirror/standard simulation, discarding their output.Per the maintainer's feedback (in learnings), the spectrograph branch should be an independent
ifbranch that doesn't execute the mirror or standard simulation at all, since it completely overwritessim_img. Currently, either the mirror ray-tracing or the standard ROI extraction runs first, then its result is discarded when the spectrograph peak is generated.Consider restructuring to:
if self._spectrograph: # independent peak simulation elif self._mirror: # mirror ray-tracing else: # standard ROI extractionThis avoids wasted computation when spectrograph mode is active.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/driver/simcam.py` around lines 434 - 457, The spectrograph code path currently runs after mirror/standard simulation and overwrites sim_img, wasting computation; change the control flow so self._spectrograph is checked first as an independent branch (if self._spectrograph: ...) that generates sim_img (using simulate_peak, x0_px, peak_center_binned, width_binned, min_val, etc.) and returns/assigns it without running the mirror ray-tracing (self._mirror) or the standard ROI extraction paths; implement the alternative branches as elif self._mirror: ... and else: ... so mirror and standard logic only run when spectrograph mode is false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/odemis/driver/simcam.py`:
- Around line 445-447: Change the diagnostic log call in simcam.py from
logging.info to logging.debug and remove the literal "DEBUG:" prefix; also
correct the format-string variable name to match the actual variable (use
roi_left, not ltrb0) so the call becomes a logging.debug(...) with the format
and arguments (x0_px, roi_left, peak_center_binned) in the same order; locate
and update the log statement near the code that computes x0_px, roi_left and
peak_center_binned (inside the related function handling peak_center_binned).
In `@src/odemis/util/synthetic.py`:
- Around line 131-141: Add explicit type hints to the simulate_peak signature:
annotate amplitude: float, x0: float, width: float, shape: Union[int, Tuple[int,
int]], and dtype: Union[numpy.dtype, Type[numpy.generic]] = numpy.uint16 (or use
numpy.typing.NDArray/ numpy.dtype types if your codebase prefers), while keeping
the existing -> numpy.ndarray return; update imports to include typing.Union,
typing.Tuple (and Type if used) or use numpy.typing aliases so the function
signature for simulate_peak(amplitude, x0, width, shape, dtype=...) is fully
typed.
- Around line 147-154: The intensity computation in synthetic.py can divide by
zero when width <= 0 and incorrectly calls numpy.iinfo for float dtypes; update
the block around x, width, intensity, numpy.iinfo, intensity_clipped and peak_1d
to first validate width (e.g., if width <= 0 raise a clear ValueError or set
width = max(width, tiny_eps) depending on desired behavior) and then choose
bounds using numpy.iinfo(dtype) for integer dtypes and numpy.finfo(dtype) for
float dtypes (use numpy.issubdtype(dtype, numpy.integer) /
numpy.issubdtype(dtype, numpy.floating) to branch), applying clipping only with
the appropriate min/max before astype to produce peak_1d.
---
Nitpick comments:
In `@src/odemis/driver/simcam.py`:
- Around line 434-457: The spectrograph code path currently runs after
mirror/standard simulation and overwrites sim_img, wasting computation; change
the control flow so self._spectrograph is checked first as an independent branch
(if self._spectrograph: ...) that generates sim_img (using simulate_peak, x0_px,
peak_center_binned, width_binned, min_val, etc.) and returns/assigns it without
running the mirror ray-tracing (self._mirror) or the standard ROI extraction
paths; implement the alternative branches as elif self._mirror: ... and else:
... so mirror and standard logic only run when spectrograph mode is false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2e93298a-0640-4bbe-95f8-5e95ec606971
📒 Files selected for processing (2)
src/odemis/driver/simcam.pysrc/odemis/util/synthetic.py
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/odemis/util/test/synthetic_test.py (2)
128-205: Good test coverage overall.The test suite covers the main functionality well. Optionally, consider adding edge case tests in the future for scenarios like
width=0(potential division by zero) orx0outside the shape bounds, to ensure robust error handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/util/test/synthetic_test.py` around lines 128 - 205, Add explicit edge-case tests to TestPeakSimulation to cover width=0 and x0 outside bounds for the simulate_peak function: create one test (e.g., test_simulate_peak_zero_width) that calls simulate_peak with width=0 and asserts either a raised exception or defined behavior (document expected behavior) and another test (e.g., test_simulate_peak_x0_out_of_bounds) that passes x0 negative and x0 >= shape and asserts proper error handling or clipping; reference the existing TestPeakSimulation class and simulate_peak function so the tests live alongside test_simulate_peak_1d/test_simulate_peak_2d and follow the same pattern (use numpy assertions and dtype/clipping checks where appropriate).
166-168: Consider using NumPy comparison for row uniformity check.The current loop is functionally correct but could be simplified with a single NumPy operation for better readability and performance.
♻️ Suggested simplification
# every row should be identical - for row in peak: - self.assertTrue(numpy.array_equal(row, peak[0])) + self.assertTrue(numpy.all(peak == peak[0]))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/util/test/synthetic_test.py` around lines 166 - 168, The loop checking each row in peak is verbose; replace it with a single NumPy comparison using numpy.all or numpy.allclose to assert uniformity. Locate the loop that iterates over peak and the use of numpy.array_equal, and change the assertion to use a vectorized check (e.g., assertTrue(numpy.all(peak == peak[0])) or assertTrue(numpy.allclose(peak, peak[0])) if values are floating-point) to improve readability and performance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/odemis/util/test/synthetic_test.py`:
- Around line 128-205: Add explicit edge-case tests to TestPeakSimulation to
cover width=0 and x0 outside bounds for the simulate_peak function: create one
test (e.g., test_simulate_peak_zero_width) that calls simulate_peak with width=0
and asserts either a raised exception or defined behavior (document expected
behavior) and another test (e.g., test_simulate_peak_x0_out_of_bounds) that
passes x0 negative and x0 >= shape and asserts proper error handling or
clipping; reference the existing TestPeakSimulation class and simulate_peak
function so the tests live alongside test_simulate_peak_1d/test_simulate_peak_2d
and follow the same pattern (use numpy assertions and dtype/clipping checks
where appropriate).
- Around line 166-168: The loop checking each row in peak is verbose; replace it
with a single NumPy comparison using numpy.all or numpy.allclose to assert
uniformity. Locate the loop that iterates over peak and the use of
numpy.array_equal, and change the assertion to use a vectorized check (e.g.,
assertTrue(numpy.all(peak == peak[0])) or assertTrue(numpy.allclose(peak,
peak[0])) if values are floating-point) to improve readability and performance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 43cf4df9-0709-4a64-b5a2-4dd2dbb093f3
📒 Files selected for processing (1)
src/odemis/util/test/synthetic_test.py
pieleric
reviewed
Mar 25, 2026
yxkdejong
force-pushed
the
add-spectrum-simulation
branch
from
5dfd1b4 to
cb519dc
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/odemis/driver/simcam.py (2)
451-457:⚠️ Potential issue | 🟠 MajorCompose the peak frame outside the sensor dtype.
simulate_peak(..., dtype=sim_img.dtype)plussim_img += peakcan wrap before the later clip when the baseline is already high, so the simulated saturation behavior becomes wrong. This block also rescans the whole frame formin()on every acquisition. Cache the baseline once during initialization (and refresh it inset_image()), compose the spectrograph frame in a wider working dtype, then clip and cast back once after noise is added.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/driver/simcam.py` around lines 451 - 457, The code currently composes the peak using the sensor dtype and recomputes the baseline via sim_img.min() on every acquisition; change this by caching the baseline/min value once during initialization (and update it in set_image()), generate the peak with a wider working dtype (e.g., float32/float64) using simulate_peak(amplitude=..., x0=peak_center_binned, width=width_binned, shape=sim_img.shape) without passing sim_img.dtype, add the peak to a working-frame created from the cached baseline in that wider dtype, apply noise, then clip to valid sensor range and cast back to sim_img.dtype before writing into sim_img (replace the existing min_val = sim_img.min(); sim_img[...] = min_val; sim_img += peak sequence). Ensure updates to baseline happen in set_image() so acquisition path no longer calls min().
434-457:⚠️ Potential issue | 🟠 MajorMake the spectrograph path exclusive.
This block still runs after the mirror/standard simulation, then immediately discards that image at Lines 454-457. Besides wasting the whole first branch, the mirror path also mutates
self._img, somxand the later noise model can depend on a frame that never reaches the caller. Please lift the spectrograph case to its own top-level branch and construct its ROI directly instead of simulating another frame first. Based on learnings,Camera._simulateshould make the spectrograph path an independent branch and computeroi_x0ascenter[0] + trans[0] + stage_shift_x - (res[0] / 2) * binning[0].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/driver/simcam.py` around lines 434 - 457, Lift the spectrograph handling in Camera._simulate into its own top-level exclusive branch that returns/uses only the spectrograph image (do not run the mirror/standard simulation first), compute the ROI x-origin as roi_x0 = center[0] + trans[0] + stage_shift_x - (res[0] / 2) * binning[0] (use stage_shift_x / stage_shift[0] as appropriate), build sim_img directly for the spectrograph path (calling simulate_peak with x0 derived from roi_x0 and width = PEAK_WIDTH / binning[0]) and avoid mutating self._img or mx in this branch so the later noise model and caller see only the intended spectrograph frame. Ensure you reference self._spectrograph, simulate_peak, sim_img, and mx when making these changes so the spectrograph branch is independent and returns its ROI image immediately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/odemis/driver/simcam.py`:
- Around line 451-457: The code currently composes the peak using the sensor
dtype and recomputes the baseline via sim_img.min() on every acquisition; change
this by caching the baseline/min value once during initialization (and update it
in set_image()), generate the peak with a wider working dtype (e.g.,
float32/float64) using simulate_peak(amplitude=..., x0=peak_center_binned,
width=width_binned, shape=sim_img.shape) without passing sim_img.dtype, add the
peak to a working-frame created from the cached baseline in that wider dtype,
apply noise, then clip to valid sensor range and cast back to sim_img.dtype
before writing into sim_img (replace the existing min_val = sim_img.min();
sim_img[...] = min_val; sim_img += peak sequence). Ensure updates to baseline
happen in set_image() so acquisition path no longer calls min().
- Around line 434-457: Lift the spectrograph handling in Camera._simulate into
its own top-level exclusive branch that returns/uses only the spectrograph image
(do not run the mirror/standard simulation first), compute the ROI x-origin as
roi_x0 = center[0] + trans[0] + stage_shift_x - (res[0] / 2) * binning[0] (use
stage_shift_x / stage_shift[0] as appropriate), build sim_img directly for the
spectrograph path (calling simulate_peak with x0 derived from roi_x0 and width =
PEAK_WIDTH / binning[0]) and avoid mutating self._img or mx in this branch so
the later noise model and caller see only the intended spectrograph frame.
Ensure you reference self._spectrograph, simulate_peak, sim_img, and mx when
making these changes so the spectrograph branch is independent and returns its
ROI image immediately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 20d55e4b-e39e-484b-8032-7cdc1c6ba85b
📒 Files selected for processing (4)
src/odemis/driver/simcam.pysrc/odemis/driver/test/simcam_test.pysrc/odemis/util/synthetic.pysrc/odemis/util/test/synthetic_test.py
✅ Files skipped from review due to trivial changes (1)
- src/odemis/util/test/synthetic_test.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/odemis/util/synthetic.py
- src/odemis/driver/test/simcam_test.py
tepals
requested changes
Mar 30, 2026
tepals
approved these changes
Apr 21, 2026
pieleric
requested changes
Apr 21, 2026
Member
pieleric
left a comment
pieleric
left a comment
There was a problem hiding this comment.
Please also have a look at code rabbit's comments.
yxkdejong
force-pushed
the
add-spectrum-simulation
branch
3 times, most recently
from
c337ec6 to
ed86791
Compare
yxkdejong
force-pushed
the
add-spectrum-simulation
branch
from
ed86791 to
6153fa7
Compare
pieleric
requested changes
Apr 24, 2026
pieleric
approved these changes
Apr 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.