Skip to content

correctness: Add degenerate calibration guard to Calibration transform #5

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Unluckyathecking merged 5 commits into from
May 25, 2026
Merged

correctness: Add degenerate calibration guard to Calibration transform #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8d0bc7a
Add degenerate calibration guards to Calibration transform
google-labs-jules[bot] May 25, 2026
8ef2103
Add degenerate calibration guards to Calibration transform
google-labs-jules[bot] May 25, 2026
7a45d81
Add degenerate calibration guards to Calibration transform
google-labs-jules[bot] May 25, 2026
b59948a
Add degenerate calibration guards to Calibration transform
google-labs-jules[bot] May 25, 2026
8ca6f0b
Merge branch 'master' into jules/exec/2026-05-25T17-52-02Z.1-10208146…
Unluckyathecking May 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/main/java/org/opensourcephysics/cabrillo/tracker/calibration/Calibration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,18 @@
}

public Calibration(double scale, double originX, double originY, double angle) {
if (!Double.isFinite(scale) || scale == 0.0) {

Check warning on line 27 in src/main/java/org/opensourcephysics/cabrillo/tracker/calibration/Calibration.java

View workflow job for this annotation

GitHub Actions / Kimi Code Review

[warning] Negative scale values accepted but likely semantically invalid 

Line 27 checks `scale == 0.0` but permits negative scale values. A negative scale (meters per pixel) would invert coordinate transformations in `toWorld` (line 54: `rx * scale`) and `toPixel` (line 66: `worldX / scale`), producing mirrored world coordinates. For a physical calibration system, scale should be strictly positive. The check should be `scale <= 0.0` instead of `scale == 0.0`, or the validation should use `scale > 0.0`.
Raw output 
        if (!Double.isFinite(scale) || scale <= 0.0) {

@github-actions github-actions Bot May 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [warning] Negative scale values accepted but likely semantically invalid

Line 27 checks scale == 0.0 but permits negative scale values. A negative scale (meters per pixel) would invert coordinate transformations in toWorld (line 54: rx * scale) and toPixel (line 66: worldX / scale), producing mirrored world coordinates. For a physical calibration system, scale should be strictly positive. The check should be scale <= 0.0 instead of scale == 0.0, or the validation should use scale > 0.0.

Suggested fix:

Suggested change
if (!Double.isFinite(scale) || scale == 0.0) {
if (!Double.isFinite(scale) || scale <= 0.0) {

throw new IllegalArgumentException("Invalid scale: " + scale);
}
if (!Double.isFinite(originX)) {

@github-actions github-actions Bot May 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 [suggestion] Negative zero scale passes validation but may be degenerate

The check scale == 0.0 allows -0.0 to pass since -0.0 == 0.0 is true in Java. However, Double.isFinite(-0.0) is also true. The downstream method toPixel divides by scale (worldX / scale and worldY / scale). Division by -0.0 produces -Infinity or Infinity (depending on sign of numerator), which is likely not intended. If the goal is to reject any zero scale, the current code actually works because scale == 0.0 catches both 0.0 and -0.0. But the error message will print -0.0 which may confuse. Consider using scale == 0.0 (already done) but documenting that both zeros are rejected, or explicitly checking Double.compare(scale, 0.0) == 0 if you want to be explicit. This is minor since the behavior is correct; flagging for awareness only.

@github-actions github-actions Bot May 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 [suggestion] Magic string pattern in exception messages

The error message prefix "Invalid scale: " and similar for other parameters use inline string literals. While the messages are clear, extracting a format pattern or using a helper would improve maintainability if error message formatting needs to change consistently across the class. This is minor given the small size of the class.

Suggested fix:

Suggested change
if (!Double.isFinite(originX)) {
private static String invalidParam(String name, double value) {
return "Invalid " + name + ": " + value;
}
public Calibration(double scale, double originX, double originY, double angle) {
if (!Double.isFinite(scale) || scale == 0.0) {
throw new IllegalArgumentException(invalidParam("scale", scale));
}

throw new IllegalArgumentException("Invalid originX: " + originX);
}
if (!Double.isFinite(originY)) {
throw new IllegalArgumentException("Invalid originY: " + originY);
}
if (!Double.isFinite(angle)) {
throw new IllegalArgumentException("Invalid angle: " + angle);
}
this.scale = scale;
this.originX = originX;
this.originY = originY;
Expand Down
41 changes: 41 additions & 0 deletions src/test/java/org/opensourcephysics/cabrillo/tracker/calibration/CalibrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,4 +82,45 @@
assertEquals(412.5, p.x(), 1e-9);
assertEquals(173.25, p.y(), 1e-9);
}




@Test
public void testDegenerateScale() {
IllegalArgumentException ex1 = assertThrows(IllegalArgumentException.class, () -> new Calibration(0.0, 0, 0, 0));
assertTrue(ex1.getMessage().contains("scale"));
IllegalArgumentException ex2 = assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.NaN, 0, 0, 0));

@github-actions github-actions Bot May 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 [suggestion] Test assertions use substring matching instead of exact validation

The degenerate tests use assertTrue(ex.getMessage().contains("scale")) which is overly permissive. If the exception message format changes to "Invalid originX: scale parameter" by mistake, the testDegenerateScale test for originX would still pass. More robust tests would verify the exact message or use a custom matcher. Additionally, the tests do not cover the -0.0 scale boundary case (which is correctly rejected by scale == 0.0, but not explicitly tested).

Suggested fix:

Suggested change
IllegalArgumentException ex2 = assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.NaN, 0, 0, 0));
IllegalArgumentException ex1 = assertThrows(IllegalArgumentException.class, () -> new Calibration(0.0, 0, 0, 0));
assertEquals("Invalid scale: 0.0", ex1.getMessage());

assertTrue(ex2.getMessage().contains("scale"));
IllegalArgumentException ex3 = assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.POSITIVE_INFINITY, 0, 0, 0));
assertTrue(ex3.getMessage().contains("scale"));
IllegalArgumentException ex4 = assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.NEGATIVE_INFINITY, 0, 0, 0));
assertTrue(ex4.getMessage().contains("scale"));

Check warning on line 98 in src/test/java/org/opensourcephysics/cabrillo/tracker/calibration/CalibrationTest.java

View workflow job for this annotation

GitHub Actions / Kimi Code Review

[warning] Weak test assertions using message substring matching 

The `testDegenerateScale` method (lines 89-98) uses `assertTrue(ex.getMessage().contains("scale"))` to verify which validation failed. This is brittle: if the `IllegalArgumentException` message format changes to `"originX must be finite, got scale: ..."` or similar, the test would still pass incorrectly. The tests should assert the exact message or use a more precise validation mechanism. Additionally, the tests do not verify that the *first* failing field is the one reported, so a bug that validates originX before scale would not be caught.
Raw output 
        IllegalArgumentException ex1 = assertThrows(IllegalArgumentException.class, () -> new Calibration(0.0, 0, 0, 0));
        assertEquals("Invalid scale: 0.0", ex1.getMessage());
        IllegalArgumentException ex2 = assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.NaN, 0, 0, 0));
        assertEquals("Invalid scale: NaN", ex2.getMessage());
        IllegalArgumentException ex3 = assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.POSITIVE_INFINITY, 0, 0, 0));
        assertEquals("Invalid scale: Infinity", ex3.getMessage());
        IllegalArgumentException ex4 = assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.NEGATIVE_INFINITY, 0, 0, 0));
        assertEquals("Invalid scale: -Infinity", ex4.getMessage());

Check warning on line 98 in src/test/java/org/opensourcephysics/cabrillo/tracker/calibration/CalibrationTest.java

View workflow job for this annotation

GitHub Actions / Kimi Code Review

[warning] Missing test for negative scale values 

The constructor at line 27 in `Calibration.java` accepts negative scale values (`scale == -2.0` passes the `scale != 0.0` check). Negative scale would cause `toWorld` to flip coordinates and `toPixel` to produce inverted results, which is likely not semantically valid for a physical calibration (meters per pixel should be positive). The tests should verify that negative scale is rejected, or the constructor should be updated to require `scale > 0.0` instead of `scale != 0.0`. Given the physical meaning of scale, `scale > 0.0` is more appropriate.

@github-actions github-actions Bot May 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [warning] Weak test assertions using message substring matching

The testDegenerateScale method (lines 89-98) uses assertTrue(ex.getMessage().contains("scale")) to verify which validation failed. This is brittle: if the IllegalArgumentException message format changes to "originX must be finite, got scale: ..." or similar, the test would still pass incorrectly. The tests should assert the exact message or use a more precise validation mechanism. Additionally, the tests do not verify that the first failing field is the one reported, so a bug that validates originX before scale would not be caught.

Suggested fix:

Suggested change
assertTrue(ex4.getMessage().contains("scale"));
IllegalArgumentException ex1 = assertThrows(IllegalArgumentException.class, () -> new Calibration(0.0, 0, 0, 0));
assertEquals("Invalid scale: 0.0", ex1.getMessage());
IllegalArgumentException ex2 = assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.NaN, 0, 0, 0));
assertEquals("Invalid scale: NaN", ex2.getMessage());
IllegalArgumentException ex3 = assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.POSITIVE_INFINITY, 0, 0, 0));
assertEquals("Invalid scale: Infinity", ex3.getMessage());
IllegalArgumentException ex4 = assertThrows(IllegalArgumentException.class, () -> new Calibration(Double.NEGATIVE_INFINITY, 0, 0, 0));
assertEquals("Invalid scale: -Infinity", ex4.getMessage());

@github-actions github-actions Bot May 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [warning] Missing test for negative scale values

The constructor at line 27 in Calibration.java accepts negative scale values (scale == -2.0 passes the scale != 0.0 check). Negative scale would cause toWorld to flip coordinates and toPixel to produce inverted results, which is likely not semantically valid for a physical calibration (meters per pixel should be positive). The tests should verify that negative scale is rejected, or the constructor should be updated to require scale > 0.0 instead of scale != 0.0. Given the physical meaning of scale, scale > 0.0 is more appropriate.

}

@Test
public void testDegenerateOrigin() {
IllegalArgumentException ex1 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, Double.NaN, 0, 0));
assertTrue(ex1.getMessage().contains("originX"));
IllegalArgumentException ex2 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, Double.NaN, 0));
assertTrue(ex2.getMessage().contains("originY"));
IllegalArgumentException ex3 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, Double.POSITIVE_INFINITY, 0, 0));
assertTrue(ex3.getMessage().contains("originX"));
IllegalArgumentException ex4 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, Double.NEGATIVE_INFINITY, 0, 0));
assertTrue(ex4.getMessage().contains("originX"));
IllegalArgumentException ex5 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, Double.POSITIVE_INFINITY, 0));

Check warning on line 111 in src/test/java/org/opensourcephysics/cabrillo/tracker/calibration/CalibrationTest.java

View workflow job for this annotation

GitHub Actions / Kimi Code Review

[warning] Weak test assertions using message substring matching 

The `testDegenerateOrigin` method (lines 100-111) uses `assertTrue(ex.getMessage().contains("originX"))` and similar substring checks. Same brittleness issue as `testDegenerateScale`: these assertions could pass for the wrong reasons if message formats change or if validation ordering bugs are introduced.
Raw output 
        IllegalArgumentException ex1 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, Double.NaN, 0, 0));
        assertEquals("Invalid originX: NaN", ex1.getMessage());
        IllegalArgumentException ex2 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, Double.NaN, 0));
        assertEquals("Invalid originY: NaN", ex2.getMessage());
        IllegalArgumentException ex3 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, Double.POSITIVE_INFINITY, 0, 0));
        assertEquals("Invalid originX: Infinity", ex3.getMessage());
        IllegalArgumentException ex4 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, Double.NEGATIVE_INFINITY, 0, 0));
        assertEquals("Invalid originX: -Infinity", ex4.getMessage());
        IllegalArgumentException ex5 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, Double.POSITIVE_INFINITY, 0));
        assertEquals("Invalid originY: Infinity", ex5.getMessage());
        IllegalArgumentException ex6 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, Double.NEGATIVE_INFINITY, 0));
        assertEquals("Invalid originY: -Infinity", ex6.getMessage());

@github-actions github-actions Bot May 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [warning] Weak test assertions using message substring matching

The testDegenerateOrigin method (lines 100-111) uses assertTrue(ex.getMessage().contains("originX")) and similar substring checks. Same brittleness issue as testDegenerateScale: these assertions could pass for the wrong reasons if message formats change or if validation ordering bugs are introduced.

Suggested fix:

Suggested change
IllegalArgumentException ex5 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, Double.POSITIVE_INFINITY, 0));
IllegalArgumentException ex1 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, Double.NaN, 0, 0));
assertEquals("Invalid originX: NaN", ex1.getMessage());
IllegalArgumentException ex2 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, Double.NaN, 0));
assertEquals("Invalid originY: NaN", ex2.getMessage());
IllegalArgumentException ex3 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, Double.POSITIVE_INFINITY, 0, 0));
assertEquals("Invalid originX: Infinity", ex3.getMessage());
IllegalArgumentException ex4 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, Double.NEGATIVE_INFINITY, 0, 0));
assertEquals("Invalid originX: -Infinity", ex4.getMessage());
IllegalArgumentException ex5 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, Double.POSITIVE_INFINITY, 0));
assertEquals("Invalid originY: Infinity", ex5.getMessage());
IllegalArgumentException ex6 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, Double.NEGATIVE_INFINITY, 0));
assertEquals("Invalid originY: -Infinity", ex6.getMessage());

assertTrue(ex5.getMessage().contains("originY"));
IllegalArgumentException ex6 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, Double.NEGATIVE_INFINITY, 0));
assertTrue(ex6.getMessage().contains("originY"));
}

@Test
public void testDegenerateAngle() {
IllegalArgumentException ex1 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, 0, Double.NaN));
assertTrue(ex1.getMessage().contains("angle"));

Check warning on line 120 in src/test/java/org/opensourcephysics/cabrillo/tracker/calibration/CalibrationTest.java

View workflow job for this annotation

GitHub Actions / Kimi Code Review

[warning] Weak test assertions using message substring matching 

The `testDegenerateAngle` method (lines 113-120) uses `assertTrue(ex.getMessage().contains("angle"))` for validation. Same brittleness issue as other degenerate tests.
Raw output 
        IllegalArgumentException ex1 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, 0, Double.NaN));
        assertEquals("Invalid angle: NaN", ex1.getMessage());
        IllegalArgumentException ex2 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, 0, Double.POSITIVE_INFINITY));
        assertEquals("Invalid angle: Infinity", ex2.getMessage());
        IllegalArgumentException ex3 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, 0, Double.NEGATIVE_INFINITY));
        assertEquals("Invalid angle: -Infinity", ex3.getMessage());

@github-actions github-actions Bot May 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [warning] Weak test assertions using message substring matching

The testDegenerateAngle method (lines 113-120) uses assertTrue(ex.getMessage().contains("angle")) for validation. Same brittleness issue as other degenerate tests.

Suggested fix:

Suggested change
assertTrue(ex1.getMessage().contains("angle"));
IllegalArgumentException ex1 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, 0, Double.NaN));
assertEquals("Invalid angle: NaN", ex1.getMessage());
IllegalArgumentException ex2 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, 0, Double.POSITIVE_INFINITY));
assertEquals("Invalid angle: Infinity", ex2.getMessage());
IllegalArgumentException ex3 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, 0, Double.NEGATIVE_INFINITY));
assertEquals("Invalid angle: -Infinity", ex3.getMessage());

IllegalArgumentException ex2 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, 0, Double.POSITIVE_INFINITY));
assertTrue(ex2.getMessage().contains("angle"));
IllegalArgumentException ex3 = assertThrows(IllegalArgumentException.class, () -> new Calibration(1.0, 0, 0, Double.NEGATIVE_INFINITY));
assertTrue(ex3.getMessage().contains("angle"));
}
}
Loading