Enhance unit test coverage #2031

oruebel · 2025-02-06T19:35:14Z

Motivation

Add unit tests to enhance overall test coverage for

Added unit tests for missed logic in core.py, image.py, spec.py, icephys.py, epoch.py
Fixed bug in IntracellularRecordingsTable, the IntracellularResponseTable was not being initialized correctly when empty category tables were provided
Fixed shape check in SpikeEventSeries to handle the case where AbstractDataChunkIterator are used for timestamps/data

Checklist

Did you update CHANGELOG.md with your changes?
Have you checked our Contributing document?
Have you ensured the PR clearly describes the problem and the solution?
Is your contribution compliant with our coding style? This can be checked running ruff check . && codespell from the source directory.
Have you checked to ensure that there aren't other open Pull Requests for the same change?
Have you included the relevant issue number using "Fix #XXX" notation where XXX is the issue number? By including "Fix #XXX" you allow GitHub to close issue #XXX when the PR is merged.

codecov · 2025-02-06T20:15:26Z

Codecov Report

Attention: Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.

Project coverage is 94.22%. Comparing base (bc12931) to head (a06e9d4).

Files with missing lines	Patch %	Lines
src/pynwb/ecephys.py	80.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #2031      +/-   ##
==========================================
+ Coverage   91.73%   94.22%   +2.48%     
==========================================
  Files          27       27              
  Lines        2722     2718       -4     
  Branches      710      708       -2     
==========================================
+ Hits         2497     2561      +64     
+ Misses        149       94      -55     
+ Partials       76       63      -13

Flag	Coverage Δ
integration	`73.14% <27.27%> (+0.18%)`	⬆️
unit	`84.76% <90.90%> (+2.47%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

oruebel · 2025-02-07T05:00:10Z

@stephprince should this PR target the dev or the release branch for the 3.0 release?

stephprince · 2025-02-07T17:37:51Z

@stephprince should this PR target the dev or the release branch for the 3.0 release?

Since you mentioned there are breaking changes, I think this should target the 3.0 release branch. But I can review and take a closer look.

oruebel · 2025-02-07T17:44:36Z

I think this should target the 3.0 release branch. But I can review and take a closer look.

Sounds good. Feel free to modify the PR as necessary.

If you prefer, it should be pretty straight forward to just move the breaking changes to a separate PR since its just removes a few functions that were no longer used anywhere.

…allowed for read of old data

…tom categories

…e_idx_count

oruebel · 2025-02-07T22:41:49Z

@stephprince @rly to simplify things I have moved all breaking changes to a new PR #2036. This PR is good to go from my end.

stephprince · 2025-02-24T20:22:07Z

src/pynwb/ophys.py

@@ -338,7 +338,7 @@ def add_roi(self, **kwargs):
        """Add a Region Of Interest (ROI) data to this"""
        pixel_mask, voxel_mask, image_mask = popargs('pixel_mask', 'voxel_mask', 'image_mask', kwargs)
        if image_mask is None and pixel_mask is None and voxel_mask is None:
-            raise ValueError("Must provide 'image_mask' and/or 'pixel_mask'")
+            raise ValueError("Must provide at least on of 'image_mask', 'pixel_mask', or 'voxel_mask'")


Suggested change

raise ValueError("Must provide at least on of 'image_mask', 'pixel_mask', or 'voxel_mask'")

raise ValueError("Must provide at least one of 'image_mask', 'pixel_mask', or 'voxel_mask'")

stephprince · 2025-02-24T20:34:27Z

src/pynwb/core.py

@@ -141,12 +154,18 @@ def __init__(self, **kwargs):

    @property
    def notes(self):
+        """
+        Get the notes attribute. This will be deprecated in the future.


Suggested change

Get the notes attribute. This will be deprecated in the future.

Get the notes attribute. Use of ScratchData.notes has been deprecated and will be removed in PyNWB 4.0.

stephprince · 2025-02-24T20:34:49Z

src/pynwb/core.py

        warn('Use of ScratchData.notes will be deprecated. Use ScratchData.description instead.',
             PendingDeprecationWarning)
        return self.description

    @notes.setter
    def notes(self, value):
+        """
+        Set the notes attribute. This will be deprecated in the future.


Suggested change

Set the notes attribute. This will be deprecated in the future.

Set the notes attribute. Use of ScratchData.notes has been deprecated and will be removed in PyNWB 4.0.

stephprince · 2025-02-24T20:58:51Z

tests/unit/test_ophys.py

@@ -558,6 +576,13 @@ def test_init_3d_image_mask(self):
        self.assertTrue(np.allclose(pS['image_mask'][0], img_masks[0]))
        self.assertTrue(np.allclose(pS['image_mask'][1], img_masks[1]))

+    def test_add_roi_missing_params(self):
+        _, _, pS = self.create_basic_plane_segmentation()
+        msg = "Must provide at least on of 'image_mask', 'pixel_mask', or 'voxel_mask'"


Suggested change

msg = "Must provide at least on of 'image_mask', 'pixel_mask', or 'voxel_mask'"

msg = "Must provide at least one of 'image_mask', 'pixel_mask', or 'voxel_mask'"

stephprince · 2025-02-24T21:35:15Z

tests/unit/test_epoch.py

+    def test_add_interval_tags_string(self):
+        """Test adding interval with tags as comma-separated string"""
+        epochs = TimeIntervals(name='test_epochs')
+        epochs.add_interval(start_time=10.0, stop_time=20.0, tags='tag1, tag2, tag3')


Should tags be allowed to be supplied this way? It feels like this approach could be prone to misunderstandings on how the data gets written.

Looking at the line of code that this is testing and the tests that fail if you remove it:

pynwb/src/pynwb/epoch.py

Line 48 in 5564f82

tags = [s.strip() for s in tags.split(",") if not s.isspace()]

it seems this line is currently mostly important for converting the tags argument to a list if a single string is provided. (This issue might be also related to hdmf-dev/hdmf#1211, where we discussed deprecating the ability to provide a single string for a ragged column.)

oruebel marked this pull request as draft February 6, 2025 22:51

oruebel marked this pull request as ready for review February 7, 2025 04:56

oruebel requested review from rly and stephprince February 7, 2025 04:56

oruebel changed the title ~~Enhance unit test for core.py~~ Enhance unit test coverage Feb 7, 2025

oruebel mentioned this pull request Feb 7, 2025

Del unused funcs #2036

Open

6 tasks

oruebel added 20 commits February 7, 2025 14:17

Add missing tests for scratch

7842987

Add missing tests for core

da1cbce

Fix ruff and some test failures

a69307b

Fix test failures

cf6ed3c

Fix test error

cfc36c0

Add missing docstrings in core

8b0ae2a

Add missing tests for IndexSeries constructor

31e30c0

Exclude ImageMaskSeries constructor from coverage because it is only …

1cc20d1

…allowed for read of old data

Fix failing test

b84e8e0

Add missing tests for NWBLinkSpec

3b89708

Add missing NWBDtypeSpec test and fix NWBLinkSpecTest

40afed3

Remove bad unit test for LinkSpec

41178ba

Add missing unit tests for NWBGroupSpec

d3798fc

Fix broken GroupSpec tests

5c75be0

Fix broken GroupSpec tests

65aac18

Add missing unit testo for ImageSegmentation.add_segmentation

2f2dde8

Add missing test for PlaneSegementation.__init__ without a name

0b813e4

Add missing unit test for PlaneSegementation.add_roi

dff1ccd

Update names of PlaneSegmentation tests

c8878a0

Add missing tests for IntracellularRecordingsTables.__init__ with cus…

1fed185

…tom categories

oruebel added 10 commits February 7, 2025 14:18

Fix bad import

65147be

Fix bug in icephys tests

6c56743

Attempt to fix test error

e1231e6

Fix ruff

e5e4254

Fix bug in IntracellularRecordingsTable

461af3c

Fix broken icephys test

e14d1d9

Add misssing unit tests for TimeIntervals.add_interval and __calculat…

ba90342

…e_idx_count

Fix validation check in SpikeEventSeries

22b3025

Update error check in SpikeEventSeries to handle DataChunkIterator

f09ad42

Add missing unit tests for ImageSeries.bits_per_pixel

eba14a8

oruebel force-pushed the enh_unittest_core branch from 68deffc to eba14a8 Compare February 7, 2025 22:20

oruebel added 2 commits February 7, 2025 14:23

Add pragma nocover for line that is hard to test

2feb09f

Update changelog

a06e9d4

stephprince reviewed Feb 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance unit test coverage #2031

Enhance unit test coverage #2031

oruebel commented Feb 6, 2025 •

edited

Loading

codecov bot commented Feb 6, 2025 •

edited

Loading

oruebel commented Feb 7, 2025

stephprince commented Feb 7, 2025

oruebel commented Feb 7, 2025 •

edited

Loading

oruebel commented Feb 7, 2025

stephprince Feb 24, 2025

stephprince Feb 24, 2025

stephprince Feb 24, 2025

stephprince Feb 24, 2025

stephprince Feb 24, 2025

	raise ValueError("Must provide at least on of 'image_mask', 'pixel_mask', or 'voxel_mask'")
	raise ValueError("Must provide at least one of 'image_mask', 'pixel_mask', or 'voxel_mask'")

	Get the notes attribute. This will be deprecated in the future.
	Get the notes attribute. Use of ScratchData.notes has been deprecated and will be removed in PyNWB 4.0.

	Set the notes attribute. This will be deprecated in the future.
	Set the notes attribute. Use of ScratchData.notes has been deprecated and will be removed in PyNWB 4.0.

	msg = "Must provide at least on of 'image_mask', 'pixel_mask', or 'voxel_mask'"
	msg = "Must provide at least one of 'image_mask', 'pixel_mask', or 'voxel_mask'"

Enhance unit test coverage #2031

Are you sure you want to change the base?

Enhance unit test coverage #2031

Conversation

oruebel commented Feb 6, 2025 • edited Loading

Motivation

Checklist

codecov bot commented Feb 6, 2025 • edited Loading

Codecov Report

oruebel commented Feb 7, 2025

stephprince commented Feb 7, 2025

oruebel commented Feb 7, 2025 • edited Loading

oruebel commented Feb 7, 2025

stephprince Feb 24, 2025

Choose a reason for hiding this comment

stephprince Feb 24, 2025

Choose a reason for hiding this comment

stephprince Feb 24, 2025

Choose a reason for hiding this comment

stephprince Feb 24, 2025

Choose a reason for hiding this comment

stephprince Feb 24, 2025

Choose a reason for hiding this comment

oruebel commented Feb 6, 2025 •

edited

Loading

codecov bot commented Feb 6, 2025 •

edited

Loading

oruebel commented Feb 7, 2025 •

edited

Loading