Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add NWB support III - provenance writing #17

Merged
merged 2 commits into from
Sep 17, 2022

Conversation

h-mayorquin
Copy link
Contributor

Description

After #14 which makes provenance data available the next step is to write this data (such as the sleap version) to the nwb file. The main purpose of this PR is to do this. I added one test to ensure that the data is properly propagated by default and another one to ensure that default behavior can be overwritten through the use of pose_estimation_metadata.

There are other two minor additions of this PR:

  1. I added tracking data to the fixture added on Add tests and functionality for loading provenance data #14 (using your tutorial and just copying the provenance data and adding tracking information). So, you will see changes of the previously added fixture. I think this is a more complete fixture for testing has it has both provenance and tracking information with your newest version of sleap. Let me know if I am missing something.
  2. I did some small refactoring to the changes introduced on Add NWB support II #15 and also added a docstring for the pose_estimation_metadata argument.

Types of changes

  • Bugfix
  • New feature
  • Refactor / Code style update (no logical changes)
  • Build / CI changes
  • Documentation Update
  • Other (explain)

Does this address any currently open issues?

Outside contributors checklist

  • Review the guidelines for contributing to this repository
  • Add tests that prove your fix is effective or that your feature works
  • Add necessary documentation (if appropriate)

Thank you for contributing to SLEAP-IO!

❤️

@h-mayorquin h-mayorquin changed the title Add NWB support III - provenance writting Add NWB support III - provenance writing Sep 14, 2022
@codecov
Copy link

codecov bot commented Sep 14, 2022

Codecov Report

Merging #17 (e44cd88) into main (491a1b3) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main       #17   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines          518       526    +8     
=========================================
+ Hits           518       526    +8     
Impacted Files Coverage Δ
sleap_io/io/nwb.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

# Propagate video metadata
default_metadata["original_videos"] = [f"{video.filename}"] # type: ignore
default_metadata["labeled_videos"] = [f"{video.filename}"] # type: ignore

Copy link
Contributor Author

@h-mayorquin h-mayorquin Sep 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was getting error sleap_io/io/nwb.py:201: error: Incompatible types in assignment (expression has type "List[str]", target has type "str") in liens 200 and 201. Is there are more elegant way of avoiding this? I guess I have to define the dict above in a specific way but I expect an heterogeneous input for each of the keys.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, yeah, the typing for complicated dict value types is a bit hairy. Technically I suppose this is intended to discourage the use of dicts as generic grab bags of different data structures that aren't documented explicitly, but it sacrifices a lot of flexibility.

Ignoring is fine for now. In my view, typing is a "best effort" practice and should be ignored if it makes the code harder to read or iterate on.

Copy link
Contributor

@talmo talmo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@talmo talmo merged commit 7f3dd13 into talmolab:main Sep 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants