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

ImageSeriesWidget fixes #196

Open
wants to merge 56 commits into
base: master
Choose a base branch
from
Open

ImageSeriesWidget fixes #196

wants to merge 56 commits into from

Conversation

Saksham20
Copy link
Contributor

Updating the ImageSeriesWidget and making TwoPhotonSeriesWidget as a child class.

  1. Changes to ImageSeriesWidget:
  • in case of external_file:
    • Formats supported: Tif > [Tif, mp4, avi.. videos]
    • Creates an additional widget which helps the user choose which of the file from the list of external_file needs to be viz.
  1. Changes to TwoPhotonSeriesWidget:
  • Inherits from ImageSeriesWidget

@Saksham20
Copy link
Contributor Author

widgets_ImageSeries_demo

@Saksham20 Saksham20 changed the title Two photon series fixes ImageSeriesWidget fixes Jan 5, 2022
@codecov
Copy link

codecov bot commented Jan 5, 2022

Codecov Report

Merging #196 (fe7d182) into master (b2fee99) will increase coverage by 1.81%.
The diff coverage is 88.14%.

❗ Current head fe7d182 differs from pull request most recent head 1d131bf. Consider uploading reports for the commit 1d131bf to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #196      +/-   ##
==========================================
+ Coverage   67.72%   69.54%   +1.81%     
==========================================
  Files          32       33       +1     
  Lines        2947     3067     +120     
==========================================
+ Hits         1996     2133     +137     
+ Misses        951      934      -17     
Flag Coverage Δ
unittests 69.54% <88.14%> (+1.81%) ⬆️

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

Impacted Files Coverage Δ
nwbwidgets/image.py 82.35% <87.36%> (+31.79%) ⬆️
nwbwidgets/utils/imageseries.py 88.29% <88.29%> (ø)
nwbwidgets/ophys.py 83.00% <100.00%> (+2.37%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2fee99...1d131bf. Read the comment docs.

nwbwidgets/image.py Outdated Show resolved Hide resolved
nwbwidgets/image.py Outdated Show resolved Hide resolved
nwbwidgets/image.py Outdated Show resolved Hide resolved
nwbwidgets/image.py Outdated Show resolved Hide resolved
nwbwidgets/image.py Outdated Show resolved Hide resolved
nwbwidgets/image.py Outdated Show resolved Hide resolved
imageseries.starting_time
+ get_frame_count(path_ext_file) / imageseries.rate
)
tmin = 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

why reset tmin to 0 every update?

Copy link
Contributor Author

@Saksham20 Saksham20 Jan 11, 2022

Choose a reason for hiding this comment

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

knowing the starting_time for video files other than the first one is not possible. Unless we assume the starting time to the end time for the last video in the sequence. But I don't think this would be a good approximation, so I have made the time_slider widget to span the video frames (0, num_frames) instead, only when the user requests video other than the first one. What do you think of this approach?
Check here https://github.com/NeurodataWithoutBorders/nwb-jupyter-widgets/blob/3add69ade68d62a7790abeac45f538e11c7ea7dc/nwbwidgets/image.py#L114

self.controls.update({key: widgets.fixed(val) for key, val in kwargs.items()})
def get_children(self, *widgets):
set_widgets = [wid for wid in widgets if wid is not None]
return [self.figure, self.time_slider, *set_widgets]
Copy link
Collaborator

Choose a reason for hiding this comment

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

a foreign time slider should not be included in the children

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case there is a foreign_time_slider, then what is the best way to add another callback (update figure) ? The usual way of using foreign_time_slider.observe(set_figure) would overwrite the old callbacks.

One possible solution would be to link the values of foreign_time_slider to the new time_slider: widgets.jslink((foreign_time_slider,"value"),(time_slider,"value")) In effect, the time_slider would now be controlled by the foreign slider while keeping the functionality of being changed independently.

Check ae8e9cc

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.

2 participants