search

LeiaInc / holopix50k

Holopix50k: A Large-Scale In-the-wild Stereo Image Dataset
Other
151 stars 11 forks source link

CV-1117 add script to visualize sample left right holopix50k images #3

Closed pritish-leia closed 4 years ago

pritish-leia commented 4 years ago

Added visualizer notebook for Holopix50k.

The Holopix50kVisualizer uses a Holopix50kDownloader object to temporarily download an image pair in images/. You can view the image pair either animated in an alternative way or placed side by side. You can also chose which Holopix50k split to download the image pair from.

PuneetKohli commented 4 years ago

The ipython notebook is not showing up in diff-view due to the large size. On downloading and playing around with it..

pritish-leia commented 4 years ago

Overall LGTM. A question from me is why we use __call__() here?

@OwenHua666 So if we repeatedly want to visualize image pairs, we need to save its state (downloader, save path) in the class. And since some visualizer.visualize() would have been the only callable function and the primary function of the class, I decided to use __call__ here instead.

OwenHua666 commented 4 years ago

Sounds good.

On Mon, Mar 23, 2020 at 19:23 pritish-leia notifications@github.com wrote:

Overall LGTM. A question from me is why we use call() here?

@OwenHua666 https://github.com/OwenHua666 So if we repeatedly want to visualize image pairs, we need to save its state (downloader, save path) in the class. And since some visualizer.visualize() would have been the only callable function and the primary function of the class, I decided to use call here instead.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/LeiaInc/holopix50k/pull/3#issuecomment-602970696, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGDP2542UW7QHXVAHP6OJJTRJAKTVANCNFSM4LSJ2WZA .

pritish-leia commented 4 years ago

The ipython notebook is not showing up in diff-view due to the large size. On downloading and playing around with it..

  • script assumes we have the IDs in the previous folder. We should clarify this assumption.
  • _read_holopix50k_ids, find_split_from_id, and download_image_pair require the docstring comments
  • For the visualizer, it is unclear that the default mode is animated. Perhaps we can specify MODE=side_by_side|animated instead of side_by_side=T|F

@PuneetKohli Will make the changes.

PuneetKohli commented 4 years ago

I am fine to merge this in, and we can review the changes as an additonal PR (so that it will show up in Github UI as well) 👍

OwenHua666 commented 4 years ago

The ipython notebook is not showing up in diff-view due to the large size. On downloading and playing around with it..

  • script assumes we have the IDs in the previous folder. We should clarify this assumption to the reader as a pre-requisite.
  • _read_holopix50k_ids, find_split_from_id, and download_image_pair require the docstring comments
  • For the visualizer, it is unclear that the default mode is animated. Perhaps we can specify MODE=side_by_side|animated instead of side_by_side=T|F

We can also make the path of the ids files as the class arguments with defaults for better generosity.

pritish-leia commented 4 years ago

The ipython notebook is not showing up in diff-view due to the large size. On downloading and playing around with it..

  • script assumes we have the IDs in the previous folder. We should clarify this assumption to the reader as a pre-requisite.
  • _read_holopix50k_ids, find_split_from_id, and download_image_pair require the docstring comments
  • For the visualizer, it is unclear that the default mode is animated. Perhaps we can specify MODE=side_by_side|animated instead of side_by_side=T|F

We can also make the path of the ids files as the class arguments with defaults for better generosity.

@OwenHua666 Didn't understand this. Right now, I have them as class arguments with defaults. Unless you meant something else.

OwenHua666 commented 4 years ago

I mean we don’t have to change those har coded parameters if we want to visualize a subset of the ids or a new set of ids.

On Mon, Mar 23, 2020 at 22:36 pritish-leia notifications@github.com wrote:

The ipython notebook is not showing up in diff-view due to the large size. On downloading and playing around with it..

  • script assumes we have the IDs in the previous folder. We should clarify this assumption to the reader as a pre-requisite.
  • _read_holopix50k_ids, find_split_from_id, and download_image_pair require the docstring comments
  • For the visualizer, it is unclear that the default mode is animated. Perhaps we can specify MODE=side_by_side|animated instead of side_by_side=T|F

We can also make the path of the ids files as the class arguments with defaults for better generosity.

@OwenHua666 https://github.com/OwenHua666 Didn't understand this. Right now, I have them as class arguments with defaults. Unless you meant something else.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/LeiaInc/holopix50k/pull/3#issuecomment-603026540, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGDP255P3FZRW5V3SF4S3ZLRJBBEXANCNFSM4LSJ2WZA .