Skip to content

Improve unit test coverage for plane and slicer geometry logic - #119

Open
Kavyashah067 wants to merge 3 commits into
brainglobe:mainfrom
Kavyashah067:improve-plane-slicer-tests
Open

Improve unit test coverage for plane and slicer geometry logic#119
Kavyashah067 wants to merge 3 commits into
brainglobe:mainfrom
Kavyashah067:improve-plane-slicer-tests

Conversation

@Kavyashah067

@Kavyashah067 Kavyashah067 commented Mar 3, 2026

Copy link
Copy Markdown
Contributor

Description

What is this PR

  • [✔️] Other
    Unit Test Coverage for the plane and slicing components of brainglobe-heatmap.

Why is this PR needed?
The slicing pipeline depends on the geometric accuracy within the plane.py and slicer.py. Improving unit test coverage for mathematical core of the code to increase the reliability and secure foundation for future enhancements or feature implementation.

What does this PR do?
This PR adds unit tests covering:

  1. Plane
  • Computation of Normal vectors
  • Validation of Orthonormal basis
  • Construction via from_norm
  • 3D -> 2D projection logic (p3_to_p2)
  • center_of_mass
  1. Slicer
  • get_ax_idx orientation handling
  • Validation of invalid orientation and position combinations
  1. No functional changes were made.
  2. Improves coverage without modifying existing functionality.

References

Brainglobe Heatmap Documentation

How has this PR been tested?

  • All tests were run locally using pytest and ensured existing implementation and unit test continue to pass.

  • Pre-commit checks passed.

  • Coverage was verified using.
    pytest --cov=brainglobe_heatmap --cov-report=term-missing

  • Coverage Impact:
    -- plane.py: 70% → 88%
    -- slicer.py: 58% → 66%
    -- Overall coverage: 71% → 74%

Is this a breaking change?

No

Does this PR require an update to the documentation?

No

Checklist:

  • [✔️] The code has been tested locally
  • [✔️] Tests have been added to cover all new functionality (unit & integration)
  • [✔️] The code has been formatted with pre-commit

@alessandrofelder alessandrofelder left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for your contribution @Kavyashah067

This looks largely good. I have some small questions and small suggestions for improvement.

Comment thread tests/test_unit/test_plane.py Outdated

plane = Plane.from_norm(origin, normal)

assert np.allclose(np.abs(plane.normal), np.array([0, 0, 1]))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need the np.abs here? If so, is this a bug in the main source code 🤔 (I would expect a .from_norm function of a plane to return a plane with (close to) the same normal.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing that out!

I’ve removed np.abs so the test now checks the direction of the normal directly.

Comment thread tests/test_unit/test_slicer.py Outdated
get_ax_idx("Frontal")


class DummyRoot:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it be possible to not use a dummy class here, and either use a mocking framework or a simple instance of the actual class instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion!

I’ve replaced the dummy class with MagicMock to better reflect real object behavior and keep the test more flexible.


plane = Plane(origin, u, v)

points_3d = np.array([[1, 2, 0], [3, 4, 0]])

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it worth testing this with points_3d that are not already in the plane?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That’s a good suggestion, thanks!

I’ve updated the test to use points that aren’t already in the plane so the projection logic is properly validated.

@codecov

codecov Bot commented Apr 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.73%. Comparing base (0fea8e1) to head (97ef16f).
⚠️ Report is 22 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #119       +/-   ##
===========================================
+ Coverage   69.63%   93.73%   +24.10%     
===========================================
  Files           5        5               
  Lines         326      367       +41     
===========================================
+ Hits          227      344      +117     
+ Misses         99       23       -76     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

2 participants