Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Initial implementation of a PoseTraining Group #9
Changes from 2 commits
783c256
d63b945
2f4248a
d763fa6
da8574b
1f1df74
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 linkssource_video
to anImageSeries
(internal or external storage) plus the frame index used and then they also specify thesource_frame
as a standaloneImage
(ideally internal or external, but see other comments regarding that). Hencesource_frame
is required, but the video it was extracted from is optional (but ideal) provenance.So should the
required
logic apply tosource_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. Ifsource_video
is provided, then because thepath
andframe_index
fields are optional, then the user does not have to provide either. From what you have both written, I think ifsource_video
is provided, thenpath
andframe_index
should also be required, and we can specify that by removingrequired: false
from both attributes.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.
Hi @CBroz1, @rly,
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"). Withsource_video
linked to anImageSeries
, I believe we may not even need thesource_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 ofImageSeries
which extendsTimeSeries
).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 hackImageSeries
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. TheIndexSeries
neurodata type supports references to anImages
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 throughImageSeries
.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 theImages
collection.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. Alsoquantity: 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.