Perf(test): load inverse-collation fixtures once at class level - #9040
Perf(test): load inverse-collation fixtures once at class level#9040aymuos15 wants to merge 2 commits into
Conversation
…ect-MONAI#9040) Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
9af7cce to
5d3d1ac
Compare
📝 WalkthroughWalkthroughThe inverse collation tests now check NIfTI availability and load reusable 2D and 3D datasets during class initialization. Each test reseeds deterministic behavior and receives shallow copies of the required datasets. Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 1
🧹 Nitpick comments (2)
tests/transforms/test_inverse_collation.py (2)
93-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Google-style docstrings to both setup hooks.
Line 94 defines
setUpClasswithout a docstring. Line 105 definessetUpwithout a docstring. Document the shared and per-test fixture attributes. Addunittest.SkipTestto theRaisessection forsetUpClass.As per path instructions, docstrings must be present for all definitions and use Google-style sections.
Also applies to: 105-105
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/transforms/test_inverse_collation.py` around lines 93 - 94, Add Google-style docstrings to the setUpClass and setUp methods in the test fixture, documenting the shared and per-test attributes they initialize. Include a Raises section in setUpClass documenting unittest.SkipTest, and ensure both docstrings use the required Google-style sections.Source: Path instructions
107-108: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winProtect the shallow-copy fixture contract.
Lines 107-108 copy only the outer dictionaries. All entries share the same loaded tensor and metadata objects.
CacheDatasetpasses source items directly to its deterministic cache stage, so an in-place transform or metadata update could contaminate later entries or tests. (raw.githubusercontent.com)Add an isolation regression check for the base fixtures, or deep-copy values when a transform requires it. Verify this contract for every transform in
TESTS_2DandTESTS_3D.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/transforms/test_inverse_collation.py` around lines 107 - 108, Update the fixture setup around data_3d and data_2d so each generated entry has isolated tensor and metadata values rather than sharing nested objects from base_3d/base_2d. Add regression coverage that checks this isolation for every transform listed in TESTS_2D and TESTS_3D, preserving the existing fixture counts and transform behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/transforms/test_inverse_collation.py`:
- Around line 100-103: Update the fixture setup around make_nifti_image and the
class initialization to retain all generated NIfTI paths in a shared temporary
directory and register class-level cleanup before creating the fixtures. Ensure
cleanup covers both files and directories, including partial failures during
base_3d or base_2d setup, while preserving the existing load_ims inputs.
---
Nitpick comments:
In `@tests/transforms/test_inverse_collation.py`:
- Around line 93-94: Add Google-style docstrings to the setUpClass and setUp
methods in the test fixture, documenting the shared and per-test attributes they
initialize. Include a Raises section in setUpClass documenting
unittest.SkipTest, and ensure both docstrings use the required Google-style
sections.
- Around line 107-108: Update the fixture setup around data_3d and data_2d so
each generated entry has isolated tensor and metadata values rather than sharing
nested objects from base_3d/base_2d. Add regression coverage that checks this
isolation for every transform listed in TESTS_2D and TESTS_3D, preserving the
existing fixture counts and transform behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 59e45b6f-f18a-4024-b487-13f60181ad2b
📒 Files selected for processing (1)
tests/transforms/test_inverse_collation.py
| im_fname, seg_fname = (make_nifti_image(i) for i in create_test_image_3d(101, 100, 107)) | ||
| cls.base_3d = load_ims({"image": im_fname, "label": seg_fname}) | ||
| im_fname, seg_fname = (make_nifti_image(i) for i in create_test_image_2d(62, 37, rad_max=10)) | ||
| load_ims = Compose([LoadImaged(KEYS), EnsureChannelFirstd(KEYS, channel_dim="no_channel")]) | ||
| self.data_2d = [load_ims({"image": im_fname, "label": seg_fname}) for _ in range(b_size)] | ||
| cls.base_2d = load_ims({"image": im_fname, "label": seg_fname}) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Clean up the temporary NIfTI files.
Lines 100 and 102 create temporary files and directories, but the returned paths are discarded. make_nifti_image assigns deletion to the caller, so each run leaves four files and directories behind. (raw.githubusercontent.com)
Retain the files in one temporary directory or register class cleanup before creating the fixtures. Cover partial setup failures too.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/transforms/test_inverse_collation.py` around lines 100 - 103, Update
the fixture setup around make_nifti_image and the class initialization to retain
all generated NIfTI paths in a shared temporary directory and register
class-level cleanup before creating the fixtures. Ensure cleanup covers both
files and directories, including partial failures during base_3d or base_2d
setup, while preserving the existing load_ims inputs.
There was a problem hiding this comment.
This is out of scope since this is pre-existing. Being tracked here now: #9041
There was a problem hiding this comment.
@aymuos15 Acknowledged. The cleanup issue is tracked in #9041, so no change is required in this pull request.
You are interacting with an AI system.
1 participant
Description
Fixtures for the inverse-collation tests were rebuilt in
setUpon every test, re-generating the synthetic NIfTI volumes 32 times per run. The build now happens once insetUpClass, with each test taking a shallow copy of the loaded dicts. The transforms never mutate their inputs and every case re-seeds determinism, so behaviour is unchanged.Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.