[fix] TestDetermineZPosition in z_localization_test.py by K4rishma · Pull Request #3408 · delmic/odemis

K4rishma · 2026-03-18T06:19:34Z

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

Copilot

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 · 2026-03-18T06:24:26Z

Warning

Rate limit exceeded

@K4rishma has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 57 minutes and 34 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 57 minutes and 34 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 594493f2-4aee-423d-87d7-d78df65bc9c4

📥 Commits

Reviewing files that changed from the base of the PR and between 20daaa0 and fc17d32.

📒 Files selected for processing (1)

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

📝 Walkthrough

Walkthrough

The test file src/odemis/acq/align/test/z_localization_test.py was updated to add an IMG_PATH constant and rewrite all image reads to use os.path.join(IMG_PATH, ...). The numeric literal z_stack_step_size was clarified from 50*10-9 to 50e-9 and PRECISION was recalculated accordingly. Several expected z values in test_determine_z_position were sign-flipped (including the limited-range re-check) and other test image path references were updated. No public APIs, function signatures, exported entities, or warning expectations 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 reflects the main change: fixing the TestDetermineZPosition test in z_localization_test.py.
Description check	✅ Passed	The description clearly relates to the changeset, mentioning the z_stack_step_size fix and the folder path addition for test 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

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.}

coderabbitai

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

tepals · 2026-03-18T13:46:48Z

        # 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]


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]

@K4rishma did you see this comment?

pieleric

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

coderabbitai

🧹 Nitpick comments (1)

src/odemis/acq/align/test/z_localization_test.py (1)
55-57: Good fix for z_stack_step_size; remove trailing empty string from IMG_PATH.

The correction from 50*10-9 (which incorrectly evaluates to 491) to 50e-9 (50 nm) properly aligns with the dataset context ("step50nm") and restores meaningful test precision.

However, the trailing "" in IMG_PATH is unconventional. While os.path.join handles it, it produces a path with a trailing separator that's unnecessary since all usages already call os.path.join(IMG_PATH, filename).
Suggested fix
-IMG_PATH = os.path.join(os.path.dirname(__file__), "images", "")
+IMG_PATH = os.path.join(os.path.dirname(__file__), "images")
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/odemis/acq/align/test/z_localization_test.py` around lines 55 - 57, The
z_stack_step_size has been corrected to 50e-9 and PRECISION is derived from it
(no change needed), but IMG_PATH contains an unnecessary trailing empty string;
update IMG_PATH by removing the trailing "" so it is built as
os.path.join(os.path.dirname(__file__), "images") (ensure existing usages that
call os.path.join(IMG_PATH, filename) still work unchanged).

🤖 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/acq/align/test/z_localization_test.py`:
- Around line 55-57: The z_stack_step_size has been corrected to 50e-9 and
PRECISION is derived from it (no change needed), but IMG_PATH contains an
unnecessary trailing empty string; update IMG_PATH by removing the trailing ""
so it is built as os.path.join(os.path.dirname(__file__), "images") (ensure
existing usages that call os.path.join(IMG_PATH, filename) still work
unchanged).

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3ba7e6f9-70fc-4d0c-9261-c19906b67e25

📥 Commits

Reviewing files that changed from the base of the PR and between 520bedf and 20daaa0.

📒 Files selected for processing (1)

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

tepals

one small thing left

tepals · 2026-04-07T08:50:27Z

-z_stack_step_size = 50*10-9  # m
+z_stack_step_size = 50e-9  # m
 PRECISION = z_stack_step_size * 0.45  # Precision should be better than the step within a z stack
+IMG_PATH = os.path.join(os.path.dirname(__file__), "images", "")


Since you are now using os.path.join everywhere, you do not need the , "" at the end

Suggested change

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

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

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

K4rishma requested review from Copilot, pieleric and tmoerkerken March 18, 2026 06:19

Copilot started reviewing on behalf of K4rishma March 18, 2026 06:20 View session

K4rishma commented Mar 18, 2026

View reviewed changes

Comment thread src/odemis/acq/align/test/z_localization_test.py Outdated

Copilot AI reviewed Mar 18, 2026

View reviewed changes

coderabbitai Bot reviewed Mar 18, 2026

View reviewed changes

Comment thread src/odemis/acq/align/test/z_localization_test.py Outdated

pieleric requested changes Mar 18, 2026

View reviewed changes

Comment thread src/odemis/acq/align/test/z_localization_test.py Outdated

Comment thread src/odemis/acq/align/test/z_localization_test.py Outdated

K4rishma force-pushed the fix_determine_z_position branch from f854ecf to 3078238 Compare March 18, 2026 11:51

K4rishma requested a review from pieleric March 18, 2026 11:52

tepals reviewed Mar 18, 2026

View reviewed changes

pieleric approved these changes Mar 26, 2026

View reviewed changes

tmoerkerken approved these changes Mar 31, 2026

View reviewed changes

K4rishma force-pushed the fix_determine_z_position branch from 3078238 to 520bedf Compare April 7, 2026 08:28

github-actions Bot added the size/M label Apr 7, 2026

K4rishma force-pushed the fix_determine_z_position branch from 520bedf to 20daaa0 Compare April 7, 2026 08:45

K4rishma requested a review from tepals April 7, 2026 08:46

coderabbitai Bot reviewed Apr 7, 2026

View reviewed changes

tepals requested changes Apr 7, 2026

View reviewed changes

[fix] TestDetermineZPosition in z_localization_test.py

fc17d32

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

K4rishma force-pushed the fix_determine_z_position branch from 20daaa0 to fc17d32 Compare April 13, 2026 11:49

tepals approved these changes Apr 13, 2026

View reviewed changes

tepals merged commit 6581901 into delmic:master Apr 13, 2026
6 checks passed

	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]

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

Conversation

K4rishma commented Mar 18, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot commented Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rate limit exceeded

Walkthrough

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tepals Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

tepals Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

pieleric left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

tepals left a comment

Choose a reason for hiding this comment

Uh oh!

tepals Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coderabbitai Bot commented Mar 18, 2026 •

edited

Loading