Skip to content

[fix] TestDetermineZPosition in z_localization_test.py#3408

Open
K4rishma wants to merge 1 commit intodelmic:masterfrom
K4rishma:fix_determine_z_position
Open

[fix] TestDetermineZPosition in z_localization_test.py#3408
K4rishma wants to merge 1 commit intodelmic:masterfrom
K4rishma:fix_determine_z_position

Conversation

@K4rishma
Copy link
Copy Markdown
Contributor

Fix the z_stack_step_size value and add a folder path for images such that test cases can be run any path.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the z-localization unit test to use an absolute path for test images (so it can run from any working directory) and adjusts the z-stack step size/precision used by the assertions.

Changes:

  • Updated z_stack_step_size and derived PRECISION for TestDetermineZPosition.
  • Introduced IMG_PATH and updated image loads to use that path instead of CWD-relative paths.
Comments suppressed due to low confidence (1)

src/odemis/acq/align/test/z_localization_test.py:114

  • This block says it changes the range so “warning 6 is raised”, but the subsequent assertion expects warning 4. Please update the comment to match the behavior being tested (or adjust the assertion if the comment is correct).
        # Change the range so warning 6 is raised with an image which is just above focus
        calib_data_limited_range = CALIB_DATA.copy()
        calib_data_limited_range["z_calibration_range"] = (-1e-10, 1e-10)
        image = read_data(IMG_PATH + "super_z_single_beed_aprox_500nm_above_focus.tif")[0]
        expected_outcome_image_3 = 420.6e-9  # Value determined using the function determine_z_position
        z, warning = determine_z_position(image, calib_data_limited_range)
        # Since the range is set to small the detected feature is to big and warning raised should be 4
        self.assertEqual(warning, 4)

image = read_data("images/super_z_no_beed_just_white.tif")[0]
image = read_data(IMG_PATH + "super_z_no_beed_just_white.tif")[0]
_, warning = determine_z_position(image, CALIB_DATA)
self.assertEqual(warning, 6) # Since the entire image is white the warning raised should be 5
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 18, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ce8f4144-f931-4fb1-9d3a-391dfa355249

📥 Commits

Reviewing files that changed from the base of the PR and between f854ecf and 3078238.

📒 Files selected for processing (1)
  • src/odemis/acq/align/test/z_localization_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/odemis/acq/align/test/z_localization_test.py

📝 Walkthrough

Walkthrough

The test file z_localization_test.py was updated to introduce an IMG_PATH constant and centralize image file references by updating all read_data calls to use IMG_PATH. A numeric literal for z_stack_step_size was clarified from 50*10-9 to 50e-9. Several test expected outcomes (signs and values in meters) were adjusted to match the new path usage and conventions. No public APIs or function signatures were changed.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main changes: fixing TestDetermineZPosition in z_localization_test.py, which aligns with the z_stack_step_size fix and image path improvements.
Description check ✅ Passed The description directly addresses the changeset by explaining both key fixes: the z_stack_step_size value correction and the addition of a folder path for images.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

You can disable sequence diagrams in the walkthrough.

Disable the reviews.sequence_diagrams setting to disable sequence diagrams in the walkthrough.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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/acq/align/test/z_localization_test.py`:
- Around line 55-56: The test's z resolution is too coarse: update the constants
so z_stack_step_size and PRECISION match the dataset ("step50nm") and production
convergence threshold (50 nm). Change z_stack_step_size from 10e-6 to 50e-9 (50
nm) and set PRECISION to that same 50e-9 (or to a slightly tighter fraction of
z_stack_step_size, e.g., 0.9 * z_stack_step_size) so the test will catch real
localization regressions; update the symbols z_stack_step_size and PRECISION
accordingly.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ce45ca74-db76-42e7-8e01-d0f7dcb2ef1d

📥 Commits

Reviewing files that changed from the base of the PR and between c1b1ff1 and f854ecf.

📒 Files selected for processing (1)
  • src/odemis/acq/align/test/z_localization_test.py

Fix the z_stack_step_size value and add a folder path for images such that test cases can be run any path.
@K4rishma K4rishma force-pushed the fix_determine_z_position branch from f854ecf to 3078238 Compare March 18, 2026 11:51
@K4rishma K4rishma requested a review from pieleric March 18, 2026 11:52
# Test on an image below focus
image = read_data("images/super_z_single_beed_aprox_500nm_under_focus.tif")[0]
expected_outcome_image_1 = -592.5e-9 # Value determined using the function determine_z_position
image = read_data(IMG_PATH + "super_z_single_beed_aprox_500nm_under_focus.tif")[0]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is easier to read if you use os.path.join here as well. So update line 57 to:

IMG_PATH = os.path.join(os.path.dirname(__file__), "images")

and then in the test cases use:

Suggested change
image = read_data(IMG_PATH + "super_z_single_beed_aprox_500nm_under_focus.tif")[0]
image = read_data(os.path.join(IMG_PATH, "super_z_single_beed_aprox_500nm_under_focus.tif"))[0]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@K4rishma did you see this comment?

Copy link
Copy Markdown
Member

@pieleric pieleric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me... assuming you implement the suggestions by Thera and copilot.

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.

5 participants