-
Notifications
You must be signed in to change notification settings - Fork 11
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
Initial implementation of a PoseTraining Group #9
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 suggestions. Added specificity regarding instances of skeletons. In all the cases I've seen, subject
would work just as well, but instance
works as more generic.
spec/ndx-pose.extensions.yaml
Outdated
- name: path | ||
dtype: text | ||
doc: Path to original video file. | ||
required: false |
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.
Should this and frame_index be optional? If a source_video is provided, it seems like a path and frame index should be provided.
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.
The logic of requirement I think was to be applied to the entirety of the source_video
attribute. Combined logic being the user either links source_video
to an ImageSeries
(internal or external storage) plus the frame index used and then they also specify the source_frame
as a standalone Image
(ideally internal or external, but see other comments regarding that). Hence source_frame
is required, but the video it was extracted from is optional (but ideal) provenance.
So should the required
logic apply to source_video
directly then?
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.
We wanted to provide two ways for supplying the training frame - either as a frame index given the path to the source video (source_video
) or as a standalone image (source_frame
). Hence why neither are required although at least one must be supplied.
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.
I think we are confusing two different things here. I get that source_video
is optional. If source_video
is provided, then because the path
and frame_index
fields are optional, then the user does not have to provide either. From what you have both written, I think if source_video
is provided, then path
and frame_index
should also be required, and we can specify that by removing required: false
from both attributes.
spec/ndx-pose.extensions.yaml
Outdated
- name: source_frame | ||
neurodata_type_inc: Image | ||
doc: Image frame used for training (stored either internally or externally). | ||
quantity: '1' |
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.
Note that currently the Image
type supports only images stored internally. Also quantity: 1
is fine to include, but it is the default when no quantity is provided, so we tend to omit it.
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.
From the past discussion, internally saving is the preferred approach since there could be thousands of external files otherwise.
However, maximal freedom for the user is also desired - would it be appropriate to extend the Image
neurodata type in this extension to allow an external mode or should we alter that in the core instead?
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.
I created an issue NeurodataWithoutBorders/nwb-schema#529 on the topic. This is part of a larger discussion over how we want to support storing external files in NWB.
These changes look good to me, and I support @CBroz1 's changes. It would be good to get feedback from @bendichter and @AlexEMG on these changes before we proceed. |
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.
@roomrys , could you look through these suggestions when you have time? This extension looks like it is really quite close to complete
spec/ndx-pose.extensions.yaml
Outdated
- name: source_frame | ||
neurodata_type_inc: Image | ||
doc: Image frame used for training (stored either internally or externally). |
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.
I believe all the changes have been address (except for Image
only allowing externally stored images and our current docs incorrectly say "stored either internally or externally"). With source_video
linked to an ImageSeries
, I believe we may not even need the source_frame
group as we can directly supply a collection of images. However, the images supplied for training are rarely in a sequential format (and may therefore be an incorrect use of ImageSeries
which extends TimeSeries
).
The problem: We would like to store non-sequential images internally. ImageSeries
allows images to be stored internally, but expects them to be sequential. Image
allows images to be non-sequential, but requires them to be stored externally.
@CodyCBakerPhD had suggested making a change to the Image
datatype to allow this. Is this feasible? Is there another workaround? Should we just hack ImageSeries
for our purposes?
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.
In NWB 2.5.0, we recently revamped the Images
neurodata type, which represents a collection of images that may or may not be ordered. The IndexSeries
neurodata type supports references to an Images
type. This type seems like a better fit for the use case.
And to be clear, Image
requires images to be stored internally and does not support external storage. External storage of individual images is not currently supported, but we can add that if we need to. External storage of movies is supported through ImageSeries
.
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.
Ok, great. I swapped the Image data type for the Images data type and added an images_index
attribute to pull out a specific frame from the Images
collection.
Hi @rly, I believe all is ready! Please let me know if there is anything else that needs work. |
Hey friends, where do we stand with this PR these days @rly @bendichter? |
Hi @talmo . Sorry for the extreme delay on wrapping up this PR... I will finalize this during the developer hackathon next week. |
Let us know if there's anything we can help with! This is a key piece of functionality for us to be able to convert our hand labeled data for archival :) |
@roomrys Could you please enable edits to this PR / branch from maintainers? There should be a checkbox on the right side. I would like to give you credit for this PR if possible. |
Sorry @rly, it seems that is impossible at the moment https://github.com/orgs/community/discussions/5634 |
@rly what if you send a PR with your changes to Or we could follow the above thread's response:
😂😂 |
Sounds good. Will do.
|
It turns out that GitHub does not allow me to fork a fork of my repo. ![]() |
I will just merge this and make changes in a new PR. Thanks for the contribution @roomrys , @CBroz1 , and @CodyCBakerPhD ! |
Description
Add
PoseTraining
group to store annotations for training a pose estimator.Related Issues