fix: address PR review feedback - warning emission, cached_property compatibility, and documentation

[Copilot]

Addresses 8 review comments on PR #61 regarding improper warning emission, a critical bug in the size property, cached_property compatibility issues, and missing documentation.

Warning Emission Fixes

• datasplit.py, multidataset.py, image.py: Replaced UserWarning(...)/Warning(...) instantiation with logger.warning() calls. Previously warnings were constructed but never emitted, silently hiding validation failures and fallback behaviors.

Critical Bug Fixes

• dataset.py size property: Fixed TypeError from .items() yielding (axis, [start, stop]) tuples. Changed to .values() to properly unpack numeric bounds:

  # Before: crashes with "unsupported operand type(s) for -: 'list' and 'list'"
  return int(np.prod([stop - start for start, stop in self.bounding_box.items()]))
  
  # After: correctly unpacks [start, stop] values
  return int(np.prod([stop - start for start, stop in self.bounding_box.values()]))

• dataset.py empty() method: Fixed incompatibility with @cached_property decorator. Changed from setting _sampling_box_shape attribute to directly populating __dict__["sampling_box_shape"] to bypass property computation.

Documentation Updates

• dataloader.py module docstring: Updated stale reference from "_array" to "array" to match current @cached_property implementation.

• dataloader.py tensorstore_cache_bytes parameter: Clarified that when cache_bytes < num_workers, each worker receives minimum 1 byte (not 0, which TensorStore treats as unlimited), causing aggregate cache to exceed the requested total.

Test Coverage

• Added test_warning_when_cache_less_than_workers to verify warning emission and 1-byte minimum enforcement when cache budget is insufficient for worker count.
• Added comprehensive tests for size and bounding_box_shape properties: Three new tests (test_bounding_box_shape, test_size_property, test_size_property_with_known_dimensions) ensure the .values() fix is covered and validate that these previously untested properties work correctly. The size property bug wasn't caught by existing tests because the property was never accessed in any test or internal code path.

---

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

[Copilot]

Pull request overview

This PR addresses critical review feedback from PR #61, fixing issues with warning emission, a TypeError bug in dataset sizing, cached_property compatibility, and documentation accuracy.

Changes:

• Fixed warning emission in datasplit.py, multidataset.py, and image.py by replacing instantiated Warning objects with logger.warning() calls
• Fixed TypeError in CellMapDataset.size property by changing .items() to .values() to properly unpack bounding box numeric bounds
• Fixed CellMapDataset.empty() to be compatible with @cached_property by directly setting __dict__["sampling_box_shape"]
• Updated dataloader.py documentation to reflect current implementation (array instead of _array) and clarify tensorstore_cache_bytes behavior with minimum 1-byte allocation
• Added test coverage for cache warning emission when cache budget is insufficient for worker count

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
tests/test_dataloader.py	Added test to verify warning emission and 1-byte minimum enforcement when tensorstore_cache_bytes < num_workers
src/cellmap_data/multidataset.py	Fixed warning emission in validation_indices property to use logger.warning() instead of instantiating UserWarning
src/cellmap_data/image.py	Fixed warning emission in array property to use logger.warning() instead of instantiating Warning/UserWarning objects
src/cellmap_data/datasplit.py	Fixed warning emission in validation_datasets_combined to use logger.warning() instead of instantiating UserWarning
src/cellmap_data/dataset.py	Fixed TypeError in size property (.items() → .values()) and cached_property compatibility in empty() method
src/cellmap_data/dataloader.py	Updated documentation for module docstring and tensorstore_cache_bytes parameter to reflect current implementation

[codecov]

Codecov Report

❌ Patch coverage is 40.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.48%. Comparing base (0c19a11) to head (fa84ec9).
⚠️ Report is 10 commits behind head on improve_cache.

Files with missing lines	Patch %	Lines
src/cellmap_data/datasplit.py	0.00%	1 Missing ⚠️
src/cellmap_data/image.py	0.00%	1 Missing ⚠️
src/cellmap_data/multidataset.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@                Coverage Diff                @@
##           improve_cache      #62      +/-   ##
=================================================
+ Coverage          70.33%   70.48%   +0.15%     
=================================================
  Files                 28       28              
  Lines               2451     2450       -1     
=================================================
+ Hits                1724     1727       +3     
+ Misses               727      723       -4     

☔ View full report in Codecov by Sentry.
📢 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.

[rhoadesScholar]

@copilot The TypeError in CellMapDataset.size property (.items() where it should have been .values() to properly unpack bounding box numeric bounds) should have been caught by tests. Why wasn't it? Add tests to improve coverage of this patch.

[Copilot]

@copilot The TypeError in CellMapDataset.size property (.items() where it should have been .values() to properly unpack bounding box numeric bounds) should have been caught by tests. Why wasn't it? Ad...

The bug wasn't caught because the size property was never tested or used internally. The property exists but is not called by __len__() (which uses sampling_box_shape instead) or any other code path.

Added three new tests in commit ffb91b9:

1. test_bounding_box_shape - tests the bounding_box_shape computation
2. test_size_property - directly tests the size property and verifies it matches the expected calculation using .values()
3. test_size_property_with_known_dimensions - tests size with specific known dimensions to validate correctness

These tests now ensure the .values() fix (instead of .items()) is covered and would have caught the TypeError.