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

Overhaul IndexSeries and add "order_of_images" to Images #459

Merged
merged 34 commits into from
Apr 30, 2022
Merged

Conversation

rly
Copy link
Contributor

@rly rly commented Aug 5, 2020

Fix #458

@rly rly added this to the NWB 2.3.0 milestone Aug 17, 2020
@bendichter
Copy link
Contributor

bendichter commented Aug 24, 2020

Maybe add ImageStack ndtype, where the first dim is n_image, but has no timing info. This could be VectorData so that we could put it into a DynamicTable and add metadata

@rly
Copy link
Contributor Author

rly commented Aug 25, 2020

I added an ImageStack type and changed IndexSeries to use it. Please review. @oruebel @bendichter @ajtritt

core/nwb.base.yaml Outdated Show resolved Hide resolved
@rly rly modified the milestones: NWB 2.3.0, NWB 2.4.0 Sep 2, 2020
@rly
Copy link
Contributor Author

rly commented Feb 22, 2021

TODO: Allow Images and ImageStack in the nwbfile/stimulus group

@rly
Copy link
Contributor Author

rly commented Feb 22, 2021

TODO: consider how images are referenced in a table of image presentations (on time, off time, other metadata). Is it possible to reference an Image from a column in a TimeIntervals or other DynamicTable?

@rly
Copy link
Contributor Author

rly commented Aug 5, 2021

After discussing this issue with @oruebel, we decided to postpone resolving this until NWB 2.5.0 while we consider the different options and talk with relevant parties.

Another idea is to create an ImageReferenceSeries that has a dataset of references to Image objects, rather than use indices into an ordered dataset. We would then deprecate IndexSeries. This may be simpler.

@rly rly marked this pull request as draft August 5, 2021 20:19
@bendichter
Copy link
Contributor

@rly , @oruebel can we revisit this?

@oruebel
Copy link
Contributor

oruebel commented Apr 21, 2022

@bendichter lets revisit this PR during our next sync

@rly rly mentioned this pull request Apr 25, 2022
5 tasks
@rly rly marked this pull request as ready for review April 26, 2022 17:19
@rly rly changed the title Overhaul IndexSeries, add ImageStack, add "order" to Images Overhaul IndexSeries and add "ordered_images" to Images Apr 26, 2022
core/nwb.base.yaml Outdated Show resolved Hide resolved
@rly
Copy link
Contributor Author

rly commented Apr 26, 2022

Upon discussion with @oruebel and @bendichter, we decided to implement:

We decided not to implement the ImageStack or ImageStacks neurodata types because those data can be represented using an Images type with the ordered_images dataset. If there is a use case where storing image stack data as an N-dimensional dataset is more preferred than storing M (N-1)-dimensional datasets in a group, then we can consider adding an ImageStack type.

We could also revamp IndexSeries in a different way by creating a new ImageReferenceSeries type that has a dataset of references to Image objects, rather than use indices into an ordered dataset of references to datasets in the Images group. Those images could reside within the ImageReferenceSeries type or in a separate Images group. Then we would deprecate IndexSeries. We would probably still want to keep the addition of "ordered_images" to the Images type. To me, this seems cleaner, though perhaps more disruptive.

i.e.

neurodata_type_def: ImageReferenceSeries
neurodata_type_inc: NWBContainer  
# NOTE this is not a TimeSeries because the dtype of data is not numeric and does not have attributes conversion, resolution, offset, unit
doc: A TimeSeries-like container that represents the times at which different images were displayed or acquired. 
datasets:
  - name: data
    dtype:
      target_type: Image
      reftype: object
    dims:
    - num_times
    shape:
    - null
    doc: References to images stored in this object.
  - neurodata_type_inc: Image
    doc: Images stored in this collection.
    quantity: '+'
  - ...  # description, timestamps, etc.

As designed in this PR, the API or user must do the following to get the images used in the IndexSeries in order:

images_container = nwbfile.stimulus_templates.get("Images")
images = [images_container.ordered_images[x] for x in index_series.data[:]]

Whereas with the ImageReferenceSeries above, the API or user must do the following:

images = image_reference_series.data[:]

@oruebel and @bendichter, what do you think? I am happy to go either way on new IndexSeries vs ImageReferenceSeries. Just wanted to present this alternative since we have not discussed it previously.

core/nwb.base.yaml Outdated Show resolved Hide resolved
@rly rly changed the title Overhaul IndexSeries and add "ordered_images" to Images Overhaul IndexSeries and add "order_of_images" to Images Apr 29, 2022
@rly rly requested review from bendichter and oruebel and removed request for ajtritt April 29, 2022 23:53
@bendichter
Copy link
Contributor

looks great!

Copy link
Contributor

@oruebel oruebel left a comment

Choose a reason for hiding this comment

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

Looks good to me

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.

IndexSeries should have the unit 'N/A' and have uint32 data
4 participants