fix: Enable loading netCDF files in the napari widget

[Sparshr04]

Description

What is this PR

• Bug fix
• Addition of a new feature
• Other

Why is this PR needed?

The napari loader widget could load data from third-party formats (like DeepLabCut), but it had no option to re-load a dataset that had been processed and saved as the project's recommended netCDF file. This created a gap in the user workflow, as discussed in issue #666

What does this PR do?

This PR adds "movement (netCDF)" as a new option to the "source software" dropdown in the loader_widgets.py file. It uses xarray to open the selected .nc file, checks that the required position variable exists (showing a warning if not), and then passes the valid dataset to the existing ds_to_napari_layers function for display.

References

Related to #666

How has this PR been tested?

I have tested the code locally by running napari with these changes.

1. Launched napari.
2. Opened the movement loader widget.
3. Verified that "movement (netCDF)" is in the dropdown.
4. Selected it and loaded a .nc file that was previously saved by movement.
5. The widget successfully loaded and displayed the points and tracks layers.
6. The project's automated test suite (pytest) also passes, showing no existing functionality was broken.

Is this a breaking change?

No, this is not a breaking change. It only adds a new loader option and does not affect any of the existing loaders.

Does this PR require an update to the documentation?

Yes, the user-facing documentation for the napari widget will likely need to be updated to list "movement (netCDF)" as a supported format.

Checklist:

• The code has been tested locally
• Tests have been added to cover all new functionality
• The documentation has been updated to reflect any changes
• The code has been formatted with pre-commit

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (21c2468) to head (d05cfe9).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #689   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           34        34           
  Lines         2070      2111   +41     
=========================================
+ Hits          2070      2111   +41     

☔ 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.

[niksirbi]

Thanks for opening the draft PR @Sparshr04. I will get back to you with feedback in a few days.

[niksirbi]

Thanks for getting this rolling, @Sparshr04!

I’ve had a closer look at the PR and also discussed it briefly with other team members during the community call. Here are some high-level comments and suggestions to guide the next steps.

Disable FPS input for netCDF files

When a user selects movement (netCDF) as the source, we should probably disable the fps input field entirely. Unlike other file types, movement netCDF files already contain time coordinates—and often an fps value in their metadata.

Allowing manual fps input could create conflicts if the user’s value doesn’t match the metadata, or if the timestamps in the file are irregular. The simplest and safest solution is to make the fps field inactive whenever movement (netCDF) is selected, and re-enable it when the user switches to another file type.

Validate datasets loaded from netCDF files

We’ll need more robust validation than simply checking for the existence of the position variable (despite what I had said in the issue).

For pose datasets, both position and confidence are required; for bounding box datasets, we also need shape. These are all used downstream by the ds_to_napari_layers() function.

You can determine the dataset type via its .ds_type attribute (see dataset attributes docs).

To avoid duplicating validation logic, it’s best to reuse existing classes and functions from movement.validators.datasets.
Here’s a sketch of what that might look like:

from napari.utils.notifications import show_error
from movement.validators.datasets import (
    ValidBboxesDataset,
    ValidPosesDataset,
    _validate_dataset,
)

class DataLoader(QWidget):
    """Widget for loading movement datasets from file."""

    def _format_data_for_layers(self):
        """Extract data required for creating napari layers."""
        if self.source_software in SUPPORTED_NETCDF_FILES:
            try:
                ds = xr.open_dataset(self.file_path)
            except Exception as e:
                show_error(f"Error opening netCDF file: {e}")
                return

            ds_type = ds.attrs.get("ds_type", None)
            validator = {
                "poses": ValidPosesDataset,
                "bboxes": ValidBboxesDataset,
            }.get(ds_type)

            if validator:
                try:
                    _validate_dataset(ds, validator)
                except (ValueError, TypeError) as e:
                    show_error(
                        f"The netCDF file does not appear to be a valid "
                        f"movement {ds_type} dataset: {e}"
                    )
                    return

A couple of notes:

• I’d use show_error rather than show_warning, since invalid datasets should block further processing.
• Even though _validate_dataset is a “private” function, it’s fine to use it here temporarily; if it proves generally useful, we can make it public in a future PR.

Testing

This will likely be the trickiest part of the PR, but it's important.

Relevant tests should go in tests/test_unit/test_napari_plugin/test_data_loader_widget.py.
Here are the main cases to cover:

1. Successful loading: Ensure valid movement netCDF files load correctly. I recently added two new sample datasets for this purpose:

   • MOVE_two-mice_octagon.analysis.nc (poses)
   • MOVE_single-crab_MOCA-crab-1_linear-interp.nc (bboxes)

   You can access them is tests as:

   file_path = pytest.DATA_PATHS.get(filename)

   I would probably start by modifying existing tests , especially test_on_load_clicked_with_valid_file_path and test_on_browse_clicked_file_filters.

2. Invalid datasets: Check that appropriate errors are raised (and displayed via show_error) for invalid files. A good approach is to create a fixture first, starting from a valid netCDF file, removing a required variable, and saving it to a temporary path. You’ll find similar patterns in tests/fixtures/files.py. Once you have your fixtures defined, you can attempt loading them in tests.

3. UI behaviour: Verify that the fps field is disabled only when movement (netCDF) is selected. This will probably require mocking GUI interactions — see examples using mocker in test_data_loader_widget.py.

I hope this advice helped. There is still a long way to go, but feel free to make incremental improvements to this PR and ask for help when needed.

[Sparshr04]

Hey @niksirbi, thanks a lot for a detailed stepwise roadmap ! Here are the steps that I took to solve issues:

Disabling FPS input for netCDF files

As we discussed earlier, currently this is the approach that I have thought of (and tested the working too) — disabling the FPS for netCDF files.

I created a simple function _on_source_software_changed to handle the logic for enabling/disabling the FPS block:

def _on_source_software_changed(self, current_text: str):
    """Enable/disable the fps spinbox based on source software."""
    is_netcdf = (current_text in SUPPORTED_NETCDF_FILES)
    # Disable fps box if netCDF
    self.fps_spinbox.setEnabled(not is_netcdf)

    if is_netcdf:
        self.fps_spinbox.setToolTip(
            "FPS is read directly from the netCDF file."
        )
    else:
        self.fps_spinbox.setToolTip(
            "Set the frames per second of the tracking data.\n"
            "This just affects the displayed time when hovering over a point\n"
        )

Validate datasets loaded from netCDF files

As you mentioned, we can determine dataset type via .ds_type attribute I used it in the logic. And you were absolutely right! I should've used show_error instead of show_warning.
And yes for now temporarily, as u said even though _validate_dataset is private function, added it to the logic. Here is the complete sketch

class DataLoader(QWidget):
    """Widget for loading movement datasets from file."""
...
    def _format_data_for_layers(self):
            """Extract data required for the creation of the napari layers."""
            if self.source_software in SUPPORTED_NETCDF_FILES:
                # Add logic for netCDF files
                try:
                    ds = xr.open_dataset(self.file_path)
                except Exception as e:
                    show_error(f"Error opening netCDF file: {e}")
                    return
    
                # Get the dataset type from its attributes
                ds_type = ds.attrs.get("ds_type", None)
                validator = {
                    "poses": ValidPosesDataset,
                    "bboxes": ValidBboxesDataset,
                }.get(ds_type)
    
                if validator:
                    try:
                        # Using validate_dataset function (currently Private)
                        _validate_dataset(ds, validator)
                    except (ValueError, TypeError) as e:
                        show_error(
                            f"The netCDF file does not appear to be a valid "
                            f"movement {ds_type} dataset: {e}"
                        )
                        return
                else:
                    show_error(
                        f"The netCDF file has an unknown 'ds_type' attribute:"
                        f"{ds_type}."
                    )
                    return

Started implementation on testing phase :)

[Sparshr04]

Testing

editing in the tests/test_unit/test_napari_plugin/test_data_loader_widget.py file
I started with the most easy test issue,

1. UI behaviour : since this is the first time I am editing a test file, read a bit about mocker to fake the simulation (in our case we wanted to mock the signal from the dropdown) as u suggested but I think I can just callsetCurrentText(), which triggers the exact same logic. Seems more simple (correct me if I'm am wrong)

def test_fps_spinbox_disabled_for_netcdf(make_napari_viewer_proxy):
    """Test that the FPS spinbox is disabled for netCDF files and enabled for others."""
    data_loader_widget = DataLoader(make_napari_viewer_proxy())

    data_loader_widget.source_software_combo.setCurrentText("DeepLabCut")
    assert data_loader_widget.fps_spinbox.isEnabled() is True

    data_loader_widget.source_software_combo.setCurrentText("movement (netCDF)")
    assert data_loader_widget.fps_spinbox.isEnabled() is False

    data_loader_widget.source_software_combo.setCurrentText("SLEAP")
    assert data_loader_widget.fps_spinbox.isEnabled() is True

And yes as u mentioned about test_on_browse_clicked_file_filters function was an easy one to fix. just needed to add "movement netCDF" to the @pytest.mark.parametrize list.

@pytest.mark.parametrize(
    "source_software, expected_file_filter",
    [
        ("DeepLabCut", "*.h5 *.csv"),
        ("SLEAP", "*.h5 *.slp"),
        ("LightningPose", "*.csv"),
        ("VIA-tracks", "*.csv"),
        ("movement (netCDF)", "*.nc"),
    ],
)

2. Successful loading: Ensure valid movement netCDF files load correctly. Testing across the two new sample datasets that you added
   MOVE_two-mice_octagon.analysis.nc (poses)
MOVE_single-crab_MOCA-crab-1_linear-interp.nc (bboxes)

@pytest.mark.parametrize(
    "filename, source_software, tracks_array_shape",
    [
        (
            "VIA_single-crab_MOCA-crab-1.csv",
            "VIA-tracks",
            (35, 4),
        ),  # single individual, no keypoints (bboxes)
        (
            "VIA_multiple-crabs_5-frames_labels.csv",
            "VIA-tracks",
            (430, 4),
        ),  # multiple individuals, no keypoints (bboxes)
        (
            "SLEAP_single-mouse_EPM.predictions.slp",
            "SLEAP",
            (110910, 4),
        ),  # single individual, multiple keypoints
        (
            "DLC_single-wasp.predictions.h5",
            "DeepLabCut",
            (2170, 4),
        ),  # single individual, multiple keypoints
        (
            "DLC_two-mice.predictions.csv",
            "DeepLabCut",
            (1439976, 4),
        ),  # two individuals, multiple keypoints
        (
            "SLEAP_three-mice_Aeon_mixed-labels.analysis.h5",
            "SLEAP",
            (1803, 4),
        ),  # three individuals, one keypoint
        (
            "MOVE_two-mice_octagon.analysis.nc",
            "movement (netCDF)",
            (126000, 4),
        ),
        (
            "MOVE_single-crab_MOCA-crab-1_linear-interp.nc",
            "movement (netCDF)",
            (0, 0),
        ),
    ],
)

This was the trickiest part yet. First I'll tell you what happened and then the solution that I think might work.
After running pytest
I got the actual shape from log messages for the MOVE_two-mice_octagon.analysis.nc (poses)
but the test was still failing for MOVE_single-crab_MOCA-crab-1_linear-interp.nc (bboxes).

I think this is happening because in our /napari/loader_widgets.py ,the source_software in the dictionary SUPPORTED_BBOXES_FILES only contains "VIA-tracks" and our source_software is "movement (netCDF)" in this case. So the logic is failing here in tests/test_unit/test_napari_plugin/test_data_loader_widget.py 's test_on_load_clicked_with_valid_file_path function

if source_software in SUPPORTED_BBOXES_FILES:
        assert data_loader_widget.data_bboxes is not None
    else:
        # Only bounding boxes datasets should add bboxes data
        assert data_loader_widget.data_bboxes is None

Solution

I think we should add a boolean argument to all the test cases in @pytest.mark.parametrize list. This tells the test function what to expect. Then the logic handling seems easy to implement.
What do u think @niksirbi should I go ahead with this logic or is there a way around this issue (like a way to check the bbox param).
Appreciate your time :)

[niksirbi]

Thanks for the update @Sparshr04.

I don't see the additions you mention in the "Files changed" tab of this PR. Have you perhaps forgotten to push the commits?

I can anyway comment on some of the questions you raised. I need some more time to think about the tests for succesfully loading valid sample netCDF files. I'll get back to you on that topic later.

Disabling FPS input for netCDF files

Your approach with _on_source_software_changed is the right one, I would just make a few modifications.

Firstly I would modify _create_fps_widget in this way:

def _create_fps_widget(self):
	#### EXISTING CODE ####
	# Add a tooltip
	self.fps_default_tooltip = (
		"Set the frames per second of the tracking data.\n"
		"This just affects the displayed time when hovering over a point\n"
		"(it doesn't set the playback speed)."
	)
	self.fps_spinbox.setToolTip(self.fps_default_tooltip)
	# Connect fps spinbox with _on_source_software_changed
	self.source_software_combo.currentTextChanged.connect(
		self._on_source_software_changed,
	)
	self.layout().addRow("fps:", self.fps_spinbox)

Then, the new _on_source_software_changed method can be written as follows:

    def _on_source_software_changed(self, current_text: str):
        """Enable / Disable the fps spinbox based on source software."""
        is_netcdf = (current_text in SUPPORTED_NETCDF_FILES)
        # Disable fps box if netCDF
        self.fps_spinbox.setEnabled(not is_netcdf)

        if is_netcdf:
            self.fps_spinbox.setToolTip(
                "Frames per second (fps) will be directly read from "
                "the netCDF file attributes."
            )
        else:
            # Restore default tooltip
            self.fps_spinbox.setToolTip(self.fps_default_tooltip)

Validate datasets loaded from netCDF files

All sound good, you can just push the changes you mentioned to update this PR.

UI behaviour tests

And yes as u mentioned about test_on_browse_clicked_file_filters function was an easy one to fix. just needed to add "movement netCDF" to the @pytest.mark.parametrize list.

Nice! Good to know.

Your test_fps_spinbox_disabled_for_netcdf test would absolutely do the job, it's definitely a valid test for this case. That said, I have a different suggestion, which amounts to splitting this into two tests, to achieve better separation of concerns and to mirror the logic of our existing UI tests.

The first test usses mocking to verify the source software combo box is wired to the "handler" (the _on_source_software_changed() method). It tests: "Does changing the combo box trigger the right method?"

def test_source_software_combo_connected_to_handler(
    make_napari_viewer_proxy, mocker
):
    """Test that changing the source software combo calls the right handler."""
    mock_method = mocker.patch(
        "movement.napari.loader_widgets.DataLoader._on_source_software_changed"
    )
    data_loader_widget = DataLoader(make_napari_viewer_proxy())
    netcdf_text = "movement (netCDF)"
    data_loader_widget.source_software_combo.setCurrentText(netcdf_text)
    mock_method.assert_called_once_with(netcdf_text)

The second test calls the handler directly and tests: "Does the handler correctly update the fps spinbox state and tooltip?". It also uses parametrisation to test multiple software choices.

@pytest.mark.parametrize(
    "choice, fps_enabled, tooltip_contains",
    [
        ("movement (netCDF)", False, "directly read from the netCDF file"),
        ("SLEAP", True, "Set the frames per second of the tracking data"),
        ("DeepLabCut", True, "Set the frames per second of the tracking data"),
    ],
)
def test_on_source_software_changed_sets_fps_state(
    make_napari_viewer_proxy, choice, fps_enabled, tooltip_contains
):
    """Test that changing the source software updates the fps spinbox.
    Both the enabled/disabled state and the tooltip should be updated.
    """
    data_loader_widget = DataLoader(make_napari_viewer_proxy())

    # initial state: fps spinbox enabled with the default tooltip
    assert data_loader_widget.fps_spinbox.isEnabled()
    assert (
        data_loader_widget.fps_spinbox.toolTip()
        == data_loader_widget.fps_default_tooltip
    )

    # call the handler directly
    data_loader_widget._on_source_software_changed(choice)

    # Assert enabled state
    assert data_loader_widget.fps_spinbox.isEnabled() is fps_enabled
    # Assert tooltip content
    assert tooltip_contains in data_loader_widget.fps_spinbox.toolTip()

This pattern is more maintainable long-term, as each test has a single clear responsibility.
It is especially useful for GUI testing where the framework handles signal/slot connections automatically, and you want to verify both the wiring AND the behaviour independently.

You will notice that we already have different sub-sections in the test_data_loader_widget.py file, one labelled as "test connection between widget methods and buttons/events" and another labelled "tests for widget methods". The first test from above belongs into the former section, while the second test belongs to the latter section.

[Sparshr04]

@niksirbi Thanks for a detailed review again :)

don't see the additions you mention in the "Files changed" tab of this PR. Have you perhaps forgotten to push the commits?

By bad, I should've mentioned that I just wanted the confirmation before I push the code. Just wanted to confirm the logic and steps before I commit the changes.

UI behaviour tests

That said, I have a different suggestion, which amounts to splitting this into two tests, to achieve better separation of concerns and to mirror the logic of our existing UI tests............ This pattern is more maintainable long-term, as each test has a single clear responsibility.
It is especially useful for GUI testing where the framework handles signal/slot connections automatically, and you want to verify both the wiring AND the behaviour independently.

I agree, this is more robust and structured and aligns with the pattern in our code. I made the changes as u mentioned in the commit.

Complete Test Suit

Thanks again for all the guidance. I've now implemented the full test suite based on your feedback:

1. UI Behaviour: I refactored the UI test into two separate tests wiring and logic as you suggested.
2. Successful Loading: I parameterized test_on_load_clicked_with_valid_file_path to include the two new .nc sample files.
3. Invalid Datasets: I added a new fixture invalid_netcdf_file_missing_confidence and a test test_on_load_clicked_with_invalid_netcdf to check the error handling.

Please let me know what you think!

[niksirbi]

Thanks for implementing the changes, @Sparshr04!

Since we now have full test coverage and all CI checks are passing, I’ve marked this PR as “ready for review” to signal that it’s getting close to completion.

I’ve left a few inline comments on the current implementation; most are minor suggestions.

As for what’s still missing:

1. As discussed in the community call last Friday, it would be good to update the current value of the (disabled) FPS widget to match ds.attrs["fps"] after loading the dataset from a netCDF file. This will visually reassure users that the correct fps value has been read. It should just take a few extra lines of code, plus a corresponding test.
2. The GUI user guide (docs/source/user_guide/gui.md), particularly the “Load the tracked dataset” section, needs to be updated to reflect the added support for netCDF files.

I suggest you focus first on addressing the inline comments and implementing point 1 above. Once that’s done, I’ll handle the documentation updates (point 2) myself, as I still want to think about how to do it exactly.

Thanks again for the great progress on this — it’s shaping up really well! 👏

[Sparshr04]

Hey @niksirbi Thanks for a detailed review once again!
I have addressed all the inline comments and implemented point 1:

As discussed in the community call last Friday, it would be good to update the current value of the (disabled) FPS widget to match ds.attrs["fps"] after loading the dataset from a netCDF file. This will visually reassure users that the correct fps value has been read. It should just take a few extra lines of code, plus a corresponding test.

Here is how I have approached the logic and testing :

Well the main code change was simple

if "fps" in ds.attrs:
    self.fps_spinbox.setValue(ds.attrs["fps"])

Testing
In the tests/test_unit/test_napari_plugin/test_data_loader_widget.py file in @pytest.mark.parametrize list of test_on_load_clicked_with_valid_file_path function, we used to set the fps of all test file types to 60.

I am thinking of adding extra parameter to @pytest.mark.parametrize list called expected_fps for testing purposes. And to sync internal self.fps variable with the UI, had to add this line

self.fps = self.fps_spinbox.value()

Let me know what you think :)

[niksirbi]

Thanks for taking care of all the comments @Sparshr04 !

I agree with your implementation of the fps updating.

Regarding the test, adding an extra parameter to test_on_load_clicked_with_valid_file_path (as you have done) totally works. That said, I'm worried that this test is starting to get out of hand. It's already quite large and hard to understand. I think my personal preference would be to test the fps behaviour in a separate unit test, like this:

@pytest.mark.parametrize(
    "filename, source_software, set_fps, expected_fps",
    [
        # For netCDF files, fps should be read from file metadata
        (
            "MOVE_two-mice_octagon.analysis.nc",
            "movement (netCDF)",
            1.0,
            50.0,  # fps from file overwrites set_fps
        ),
        # For non-netCDF files, fps should be the value set by the user
        (
            "DLC_single-wasp.predictions.h5",
            "DeepLabCut",
            30.0,
            30.0,  # set_fps persists
        ),
    ],
    ids=["netcdf_file", "dlc_file"],
)
def test_fps_handling_on_load(
    filename,
    source_software,
    set_fps,
    expected_fps,
    make_napari_viewer_proxy,
):
    """Test that FPS is correctly handled when loading files.

    For netCDF files (.nc), the FPS should be read from file metadata.
    For all other file types, the FPS should be the value set in the spinbox.
    """
    # Instantiate the napari viewer and the data loader widget
    viewer = make_napari_viewer_proxy()
    data_loader_widget = DataLoader(viewer)

    # Set the file path
    file_path = pytest.DATA_PATHS.get(filename)
    data_loader_widget.file_path_edit.setText(file_path.as_posix())

    # Set the source software
    data_loader_widget.source_software_combo.setCurrentText(source_software)
    # Set the fps spinbox to the desired value
    data_loader_widget.fps_spinbox.setValue(set_fps)

    # Load the file
    data_loader_widget._on_load_clicked()

    # Check that the fps attribute matches the expected value
    assert data_loader_widget.fps == expected_fps
    # Check that the value of the fps spinbox has also been updated
    assert data_loader_widget.fps_spinbox.value() == expected_fps

I think that the above 2 test cases are sufficient for now, no need to go overboard. Let me know what you think. I do realise that some of my testing preferences are a matter of taste.

Apart from that, I'm approving this PR as it's definitely good enough. I will try to find a bit of time in the next days to document this feature, and after that I'll merge it.

[niksirbi]

I'm pre-approving this PR. Docs are missing, but I will take care of that myself before merging.

[Sparshr04]

Thanks a lot for the review and feedback @niksirbi !

Regarding the test, adding an extra parameter to test_on_load_clicked_with_valid_file_path (as you have done) totally works. That said, I'm worried that this test is starting to get out of hand. It's already quite large and hard to understand. I think my personal preference would be to test the fps behaviour in a separate unit test

I completely agree, makes things cleaner and easier to maintain. To be honest I was gonna suggest for the same approach. Over the time we have made the code more readable and structured in the tests. I mean why leave a stone unturned :)

I’ll proceed with this direction and keep the structure simple and readable as we discussed.
Also, thanks for approving the PR! Really appreciate the guidance throughout this.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud