I/O support for the ndx-pose NWB extension: take 2

[niksirbi]

Description

What is this PR

• Bug fix
• Addition of a new feature
• Other

Why is this PR needed?

This PR is the continuation of #166, building on @edeno's hard work over there. See that PR for more information.

What does this PR do?

Adds a new I/O module, that handles conversions between ndx-pose and movement poses datasets.

References

Closes #23

How has this PR been tested?

TBD...

Is this a breaking change?

Probably not...

Does this PR require an update to the documentation?

Yes, supported formats need to be updated.

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

Attention: Patch coverage is 85.36585% with 12 lines in your changes missing coverage. Please review.

Project coverage is 99.14%. Comparing base (d2cd4e6) to head (fb48ea5).

Files with missing lines	Patch %	Lines
movement/io/nwb.py	83.33%	7 Missing ⚠️
movement/io/load_poses.py	86.66%	4 Missing ⚠️
movement/io/save_poses.py	90.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #360      +/-   ##
==========================================
- Coverage   99.87%   99.14%   -0.73%     
==========================================
  Files          28       29       +1     
  Lines        1559     1641      +82     
==========================================
+ Hits         1557     1627      +70     
- Misses          2       14      +12     

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

[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

[luiztauffer]

it is actually quite common to use the rate parameter instead of timestamps, but of course both situations could happen. I'd suggest this:

Suggested change

	fps = np.nanmedian(1 / np.diff(pse.timestamps))
	if pse.rate:
	fps = pse.rate
	else:
	fps = np.nanmedian(1 / np.diff(pse.timestamps))

[niksirbi]

Thanks, I didn't know this!

[luiztauffer]

hi @niksirbi , thanks for working on this!
since reading from NWB is a bit more straightforward task, and it seems to be complete in this code, what do you think about merging the read part and leaving the write as a TODO for now?
I say this because, although the write to nwb code looks pretty complete, it still lacks the possibility to include derived variables (velocity, acceleration, etc...) and source videos

[luiztauffer]

we could get source videos like this:

Suggested change

	pose_estimation_series = pose_estimation.fields["pose_estimation_series"]
	pose_estimation_series = pose_estimation.fields["pose_estimation_series"]
	source_videos_paths = list(pose_estimation.original_videos[:])

[niksirbi]

For our other formats (e.g. SLEAP) we don't normally keep track of this information, as in we don't store it in the dataset's metadata. That said, since NWB makes the information so readily available, perhaps we can consider adding this as a metadata field (in the .attrs dict)?

[luiztauffer]

oh, for some reason I thought that info was present in the datasets metadata. Yeah, it sounds like it would be a good addition, at least for full provenance!

[luiztauffer]

in these NWB objects, we can use dot notation like this:

Suggested change

	source_software = pose_estimation.fields["source_software"]
	source_software = pose_estimation.source_software

[niksirbi]

hi @niksirbi , thanks for working on this! since reading from NWB is a bit more straightforward task, and it seems to be complete in this code, what do you think about merging the read part and leaving the write as a TODO for now?

Thanks for reviewing this @luiztauffer. The activity in the repo ticked up by a lot so I didn't get a chance to finalise this. What you propose here makes sense to me. It will be easier to split the load/save bits and focus on merging the loading functions first. We'd only have to improve the relevant tests a bit, but I think that won't be hard. If I don't manage to bring this to a conclusion this week I will bring it up at out community call this Friday, to see if anyone else has capacity.

I say this because, although the write to nwb code looks pretty complete, it still lacks the possibility to include derived variables (velocity, acceleration, etc...) and source videos

Does the NWB spec explicitly support derived variables or do you mean that it can be co-opted to do so?

[luiztauffer]

@niksirbi ok, cool! Let me know how it goes and maybe I can help you out =)

Does the NWB spec explicitly support derived variables or do you mean that it can be co-opted to do so?

Yes, we could add them for example as BehavioralTimeSeries objects, inside the same behavior processing module

[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

[niksirbi]

Hey @luiztauffer, just letting you know that we've discussed this internally, and decided that @lochhh will take over the remainder of the work, along the lines of what we've agreed above:

• first finalise and merge the loading part
• tackle the saving separately

I'm confident that she will be much more effective than me in wrapping this up.