-
Notifications
You must be signed in to change notification settings - Fork 49
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 multi-animal support to DLCLive #72
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor revisions, mostly to do with keeping API consistent between DLCLive
and MultiAnimalDLCLive
. To me it would be nice to have these in a flat hierarchy: root DLCLive
object, then subclasses that implement the different variations of inference -- as is it's getting a bit messy with the root object having a bunch of if
switches that are sort of mutually incompatible with the subclass. Seems like a headache to maintain, but I ain't doing it so i'm not going to sit here and demand you to do it for me ;)
The stuff that I think should be changed are
- the things that match the signatures between the two classes,
- matching behavior for
cropping
anddynamic_cropping
(by splitting them out to a separate method in the parent class rather than duplicating the code in the child class) - adding documentation (in
__init__
and any changes toget_pose
(like ifpose
is a different shape or something)
the rest are 'would be nice' and might be helpful for maintenance/readability.
pcutoff: float = 0.5, | ||
display_radius: int = 3, | ||
display_cmap: str = "bmy", | ||
): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs docstring! Since inherits from DLCLive
but the signature differs, need to also document changes in calling convention. Also need to document any new attrs
class MultiAnimalDLCLive(DLCLive): | ||
def __init__( | ||
self, | ||
model_path, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type hint!
model_path: Union[Path, str]
n_animals: int
n_multibodyparts: int
def __init__( | ||
self, | ||
model_path, | ||
n_animals, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these not specified in the config file?
model_path, | ||
n_animals, | ||
n_multibodyparts, | ||
track_method: str = "box", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could be a literal
track_method: Literal['box', 'ellipse']
display: typing.Union[bool, Display] = False, | ||
pcutoff: float = 0.5, | ||
display_radius: int = 3, | ||
display_cmap: str = "bmy", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing arguments from parent signature:
model_type
precision
tf_config
cropping
dynamic
Any reason? We should try and make uniform API if possible.
Since these are all in the DLCLive.PARAMETERS
tuple, this will cause .parameterization
to error. This also removes control over the process_frame
method.
Two approaches: add the missing arguments, or else remove all of them and accept a **kwargs
that gets passed to the parent class.
self.display.display_frame(frame, self.pose) | ||
|
||
if self.resize is not None: | ||
self.pose[0][..., :2] *= 1 / self.resize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like this should happen for n
poses, right? rather than a hard 2? but i'm not sure what the structure of pose is here.
self.pose[1][:, :2] *= 1 / self.resize | ||
|
||
if self.processor: | ||
self.pose = self.processor.process(self.pose, **kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does the processor need to be different for maDLC pose?
if self.processor: | ||
self.pose = self.processor.process(self.pose, **kwargs) | ||
|
||
return self.pose |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does the close
method need to be updated at all? again not sure how the maDLC stuff works.
raise DLCLiveError("No frame provided for live pose estimation") | ||
|
||
frame = self.process_frame(frame) | ||
data_dict = predict_multianimal.predict_batched_peaks_and_costs( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure what happens from here to L627, some comments would be nice!
|
||
return pose | ||
|
||
def get_pose(self, frame=None, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now might also be a good time to add docstrings to these methods too -- their function is straightforward enough, but documenting what goes on in them, etc. either here or documenting the arguments/attrs in the __init__
docstring that change the function here.
Hell ya. thanks for handling this. I can't push to your fork, so I made a few initial hopefully uncontroversial amendments in
|
@jeylau Are you still working on this? Otherwise I could attempt to make the requested changes. |
Hey @thomasilmer, I'll update this PR soon, but please feel free to contribute too; you'd be very welcome! |
@jeylau let's finish this :) |
Hey @jeylau, I took a look at the requested changes and must admit that I quickly realized that I am not familiar enough with the internal workings of DeepLabCut to contribute anything meaningful at this moment. My apologies. |
Hi @jeylau @MMathisLab !I'm currently trying to apply maDLC models on live videos. Just wondering what is the status of supporting maDLC models in DLCLive? I'm also willing to contribute to help the development! |
I have tried running this code locally and there were some changes I had to make to prevent exceptions from happening when indexes went out of bounds. What is the preferred method for contributing these changes @jeylau ? Additionally, I noticed that track_method='ellipse' seems to give more consistent poses than track_method='box'. Is there any reason for this or is this heavily dependent on the type of data you process? Finally, there is some strange behavior when analysing videos where animals enter the frame from one side and exit on the other side (have not tested on other footage). The animals are lost after some time and we need to run 'init_inference' again to get better results again. |
Very happy to accept a PR - so please just edit in your forked copy and open a pull request to us and tag us for review, thanks!! Cc @n-poulsen |
Hi guys @gkane26 @jeylau @MMathisLab @sneakers-the-rat , first things first, a huge thanks and shout out to the DLC team and the people who are contributing to this wonderful project in any way whatsoever. As I implemented the MultiAnimalDLCLive class, I encountered several issues that are known to you guys and the great DLC community. I'll list the issues which I have encountered below: 1 - init_inference() : When you call init_inference(allow_growth=True) before get_pose(frame), the pose estimation seems to work well. However, in the long run, the pose estimation drops drastically in frames. In some cases, it fails to detect any body parts. One workaround is to call init_inference(allow_growth=True) whenever that happens, which asks for a lot of memory, causing the frame rate to drop to zero.” 2 - max_age : It needs to be set as high as ‘100’ for my project, in order for MultiAnimalDLCLive to be able to detect body parts. Otherwise, MultiAnimalDLCLive is unable to detect body parts and the detection rate drops drastically, near to zero detections. 3 - track_method : The ‘Box’ tracking method detects far fewer body parts in frames compared to the ‘Ellipse’ tracking method |
MultiAnimalDLCLive
can be instantiated e.g. as:Fixes #57