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

Port over DLC conversion utils #946

Merged
merged 25 commits into from
Jul 11, 2024
Merged

Port over DLC conversion utils #946

merged 25 commits into from
Jul 11, 2024

Conversation

CodyCBakerPhD
Copy link
Member

@h-mayorquin and I talked about this, and it's come up before in the past...

The DLC team has historically taken far too long to even respond to issues, let alone push fixes or review PRs on dlc2nwb

So we're just porting over the util functions here so we have full control over making minor bug fixes or improvements like the recent container name exposure (and no doubt others we'll find over time)

cc @vigji I'll absorb your fixes from DeepLabCut/DLC2NWB#23 and for rebase your NeuroConv PR to this one (assuming I can)

@CodyCBakerPhD CodyCBakerPhD self-assigned this Jul 10, 2024
setup.py Show resolved Hide resolved
@vigji
Copy link
Contributor

vigji commented Jul 10, 2024

@h-mayorquin and I talked about this, and it's come up before in the past...

The DLC team has historically taken far too long to even respond to issues, let alone push fixes or review PRs on dlc2nwb

So we're just porting over the util functions here so we have full control over making minor bug fixes or improvements like the recent container name exposure (and no doubt others we'll find over time)

cc @vigji I'll absorb your fixes from DeepLabCut/DLC2NWB#23 and for rebase your NeuroConv PR to this one (assuming I can)

Sounds great, good for me ofc! Thanks!

@CodyCBakerPhD CodyCBakerPhD marked this pull request as ready for review July 11, 2024 01:34
@CodyCBakerPhD
Copy link
Member Author

@h-mayorquin The live service failures should have nothing to do with this I'd think (some out of sync release on DANDI side)

Copy link
Collaborator

@h-mayorquin h-mayorquin left a comment

Choose a reason for hiding this comment

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

OK, some questions. I will give it another read tomorrow but this seems like a simple copy-paste which should work. We can merge and then slowly improve as we need it.

from platform import python_version
from typing import List, Optional, Union

import cv2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this import be protected? I was expecting it to throw imports errors.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, sure

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch

Copy link
Member Author

Choose a reason for hiding this comment

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

But as to why it was not throwing error, it's because we don't test global importability of this private submodule and it's not triggered by anything global (the import of this submodule is itself lazy in the interface usage)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is because of the protection with __all__ on the __init__ files?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't read much about the reasoning, only that it triggered on all the ones not contained in the __all__ of that file (which is most if not all of them)

@CodyCBakerPhD CodyCBakerPhD enabled auto-merge (squash) July 11, 2024 04:53
Copy link
Collaborator

@h-mayorquin h-mayorquin left a comment

Choose a reason for hiding this comment

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

OK, LGTM.

@CodyCBakerPhD CodyCBakerPhD merged commit ddb8a86 into main Jul 11, 2024
23 of 35 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the port_dlc2nwb branch July 11, 2024 16:43
Copy link

codecov bot commented Oct 9, 2024

Codecov Report

Attention: Patch coverage is 59.28571% with 57 lines in your changes missing coverage. Please review.

Project coverage is 90.14%. Comparing base (0866c30) to head (d3ea2a1).
Report is 118 commits behind head on main.

Files with missing lines Patch % Lines
...v/datainterfaces/behavior/deeplabcut/_dlc_utils.py 58.08% 57 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #946      +/-   ##
==========================================
- Coverage   90.76%   90.14%   -0.63%     
==========================================
  Files         125      126       +1     
  Lines        7085     7214     +129     
==========================================
+ Hits         6431     6503      +72     
- Misses        654      711      +57     
Flag Coverage Δ
unittests 90.14% <59.28%> (-0.63%) ⬇️

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

Files with missing lines Coverage Δ
...ces/behavior/deeplabcut/deeplabcutdatainterface.py 92.50% <100.00%> (-1.12%) ⬇️
...v/datainterfaces/behavior/deeplabcut/_dlc_utils.py 58.08% <58.08%> (ø)

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.

3 participants