waldo-seg / waldo

image-segmentation and text-localization
Apache License 2.0
13 stars 13 forks source link

Removed train_image_size from CoreConfig #29

Closed desh2608 closed 6 years ago

desh2608 commented 6 years ago

Removed train_image_size from CoreConfig and added as argument for functions in data_manipulation and validate() for CoreConfig. Also added it as command line argument in training script for dsb2018 with default value None.

N.B: train_image_size is directly used without a None check in several places. I think we should provide a default value for it other than None otherwise there may be issues down the line. Please provide your thoughts.

danpovey commented 6 years ago

That seems to make sense to me. There might be a few utility functions that need to be switched over. I hope Yiwen can take care of this while updating the dsb2018 setup to use more of the core tools.

On Wed, May 16, 2018 at 1:06 AM, Desh Raj notifications@github.com wrote:

@desh2608 commented on this pull request.

In scripts/waldo/data_types.py https://github.com/waldo-seg/waldo/pull/29#discussion_r188504039:

 """

This function validates that 'c' is a valid configuration object of type CoreConfig. """ assert isinstance(c, CoreConfig)

  • c.validate()
  • c.validate(train_image_size)

def validate_image_with_mask(x, c):

@hhadian https://github.com/hhadian yes, Numpy and Scipy use the row-major format, while image libraries such as Pillow or Matplotlib use the column-major format. I think since we are storing all image data as numpy arrays anyway, we should use the (height, width, num_colors) format, which would avoid the need for axes transformations while converting.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/waldo-seg/waldo/pull/29#discussion_r188504039, or mute the thread https://github.com/notifications/unsubscribe-auth/ADJVuwQoC1gKBHfo9xhAuVcZe1X-tPAiks5ty7PAgaJpZM4T_I8d .

danpovey commented 6 years ago

It will be image_with_mask (since it's much smaller). Yiwen, I think you can decide on the format of image_with_mask, but make sure that your solution allows our visualization code to work.

On Thu, May 17, 2018 at 2:21 PM, Yiwen Shao notifications@github.com wrote:

@YiwenShaoStephen commented on this pull request.

In scripts/waldo/data_types.py https://github.com/waldo-seg/waldo/pull/29#discussion_r189055527:

 """

This function validates that 'c' is a valid configuration object of type CoreConfig. """ assert isinstance(c, CoreConfig)

  • c.validate()
  • c.validate(train_image_size)

def validate_image_with_mask(x, c):

Sorry I'm a little bit late for this conversation. I'm wondering what we are going to store as dataset path (i.e. train.pth.tar)? combined_image (numpy array) or imge_with_mask (a dict)? No matter what the format of image is, we need to make it a (num_channels, height, width) tensor for pytorch training.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/waldo-seg/waldo/pull/29#discussion_r189055527, or mute the thread https://github.com/notifications/unsubscribe-auth/ADJVu2yxG_A7GxTUmlQBdBACFCQRLRIZks5tzb-ogaJpZM4T_I8d .

YiwenShaoStephen commented 6 years ago

OK, I will merge these utilities in scripts and make sure they are compatible.

YiwenShaoStephen commented 6 years ago

@hhadian @desh2608 I think we'd better use (height, width, colors) for image_with_mask since it's the default format in PIL or scipy. And I will make it a (colors, height, width) numpy array in convert_to_combined_image for further network training reason. @desh2608 Can you make these changes for image_with_mask related codes? I will focus on the convert_to_combined_image stuff.

danpovey commented 6 years ago

Second this. We need to decide, and let's settle on that decision.

On Thu, May 17, 2018 at 5:18 PM, Yiwen Shao notifications@github.com wrote:

@hhadian https://github.com/hhadian @desh2608 https://github.com/desh2608 I think we'd better use (height, width, colors) for image_with_mask since it's the default format in PIL or scipy. And I will make it a (colors, height, width) numpy array in convert_to_combined_image for further network training reason. @desh2608 https://github.com/desh2608 Can you make these changes for image_with_mask related codes? I will focus on the convert_to_combined_image stuff.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/waldo-seg/waldo/pull/29#issuecomment-390014973, or mute the thread https://github.com/notifications/unsubscribe-auth/ADJVu6c881G8vbAtxhgfW9JgKwW0sCoxks5tzekYgaJpZM4T_I8d .

hhadian commented 6 years ago

This sounds good to me.

desh2608 commented 6 years ago

@YiwenShaoStephen Ok I'll make the shape modifications for the image_with_mask related code.