densenet dilation for segmentation #46
Conversation
|
@ahundt Nice work! I will be a bit busy the next few days due to midsems, but you should carry on. I will read up on the papers in the meantime. On that note, perhaps add the paper to the top of the document referenced and add some documentation with example usage in the docstring of the DenseNet class, as people may inadvertently be confused as to why DenseNet has dilation parameters when it normally doesn't use AtrousConvolutions. |
| @@ -331,6 +338,9 @@ def __transition_block(ip, nb_filter, compression=1.0, dropout_rate=None, weight | |||
| in the transition block. | |||
| dropout_rate: dropout rate | |||
| weight_decay: weight decay factor | |||
| dilation_rate: an integer or tuple/list of 2 integers, specifying the dilation rate to | |||
| use for dilated convolution. Can be a single integer to specify the same value for | |||
| all spatial dimensions. | |||
|
|
|||
There was a problem hiding this comment.
Add docstring explaining pooling parameters
There was a problem hiding this comment.
The docstring should also explain pooling parameter. Just copy paste from the DenseNet docstring.
| if pooling == "avg": | ||
| x = AveragePooling2D((2, 2), strides=(2, 2))(x) | ||
| elif pooling == "max": | ||
| x = MaxPooling2D((2, 2), strides=(2, 2))(x) | ||
|
|
There was a problem hiding this comment.
There should be neither max nor avg pooling in the None case, as that changes the dimensions and will affect the resolution of the segmentation output. The receptive field in that case is managed by the atrous convolution, not pooling.
|
Please don't open a Pull Request unless the functionality is ready to be merged, especially in a repository where multiple people have merge abilities. |
|
@patyork what's your recommended procedure for early feedback to ensure ongoing work isn't duplicated or off the rails in terms of approach? |
|
It's in CONTRIBUTING.md and is the same as for Keras: Open an issue to explain and gauge interest in/need for/feasibility of the idea and flesh out the scope of the feature; if the idea passes muster (for contrib, it most likely will) fork the repo, make a branch, implement the feature fully per any discussion in the relevant issue; open a pull request referencing the issue; make any changes as requested by reviewers; feature merged.
This is as opposed to everyone opening a PR that isn't ready to merge and: Reviewers auditing broken code, reviewing the same code many times as new "parts" get added or fixed, reviewers not knowing if something is ready for merge/review, poorly thought-out PRs that end up having 20-30 commits on which Travis runs each time, maintainers getting emails for each PR when they have no action item on it, someone spending 10 hours implementing a feature that ends up not being merged at all (bad code, unneeded or duplicate feature) or that upon review could have been done in a different way in 5 minutes, etc, etc, etc. In other words, there's good reason why CONTRIBUTING.md has already spelled all of this out.
…-----Original Message-----
From: "Andrew Hundt" <notifications@github.com>
Sent: 3/17/2017 1:58 PM
To: "farizrahman4u/keras-contrib" <keras-contrib@noreply.github.com>
Cc: "Pat York" <pat.york@nevada.unr.edu>; "Mention" <mention@noreply.github.com>
Subject: Re: [farizrahman4u/keras-contrib] [WIP] densenet dilation forsegmentation (#46)
@patyork what's your recommended procedure for early feedback then to ensure ongoing work isn't duplicated or off the rails in terms of approach?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Sorry if I've wasted your time, I know as a long time Keras contributor and with Keras being so popular you must have your inbox flooded. Is there a mistake in my understanding that github designed pull requests and issues to be essentially identical modulo associated commits? I find the travis runs are actually quite useful. I can follow this procedure in the future if I'm discussing something specific with other contributors:
It does seem slightly redundant to me and there will be two #s associated with changes and items to close, but I will live. :-) I hope you'll understand when also I mention that the contributing.md isn't yet as clear as it could be on this point:
This could be read as creating issues for changes in existing, supported functionality which would of course have a much higher bar for acceptance, especially if existing code could break. One option that may be helpful for you, in some cases consider using the unsubscribe button and devs would likely @ mention you when attention is needed? I've been using unsubscribe for some items and it isn't perfect but it does reduce the noise. Maybe we need to add a new Github issue and PR importance classifier as a Keras example and chrome extension, LOL! |
|
Notifications are just a side issue. The main points:
The other points stand, but those are my two main ones. Issues and PRs are separate entities for different purposes, and that's why they exist separately. |
|
Thanks again for the clarification, I plan to follow that approach in the future for Actually, your comments may be worth adding directly to both |
# Conflicts: # keras_contrib/applications/densenet.py
ahundt
changed the title
|
@titu1994 This is ready for review now, could you take a look? If you won't be available let me know and I will ping others. Note that the usage I have here is the way atrous convolutions work, so I've removed it from the FCNDenseNet code in 9665d72, perhaps an upscaling dilation rate of some sort could be added to that but it should be future work and would just add a dilation parameter to the already existing Conv2DTranspose. To run the code: Pascal VOC 2012 will need to be extracted in: An automated Pascal VOC configuration and download script can be found at my fork of tf-image-segmentation on the ahundt-keras branch. It can also be downloaded by hand: # original PASCAL VOC 2012
mkdir -p ~/datasets
cd ~/datasets
curl -O http://host.robots.ox.ac.uk/pascal/VOC/voc2012/VOCtrainval_11-May-2012.tar # 2 GB
tar -xvzf VOCtrainval_11-May-2012.tarMy Keras-FCN fork on the densenet_atrous branch and run the train.py script then the evaluate.py script. cd ~/src/Keras-FCN
python train.py && python evaluate.pyWhile I hope to eventually directly incorporate an FCN training script into keras-contrib, I'm waiting on permission from the author of Keras-FCN. Let me know if any additional information is needed! |
|
|
||
| if upsampling_type == 'deconv' and batchsize is None: | ||
| raise ValueError('If "upsampling_type" is deconvoloution, then a fixed ' | ||
| 'batch size must be provided in batchsize parameter.') | ||
|
|
||
| if input_shape is None: | ||
| raise ValueError('For fully convolutional models, input shape must be supplied.') | ||
| raise ValueError( | ||
| 'For fully convolutional models, input shape must be supplied.') |
There was a problem hiding this comment.
Should be fit in one line
| # If doing segmentation we still include top | ||
| # but _obtain_input_shape only supports | ||
| # labeling, not segmentation networks. | ||
| include_top=False) |
There was a problem hiding this comment.
You could wait for my bugfix PR to be merged, it removes this dependency on _obtain_input_shape and uses only those parts of it which make sense to segmentation models. If not, atleast remove the comments inlined with the method arguments and put it above the method call.
| @@ -331,6 +338,9 @@ def __transition_block(ip, nb_filter, compression=1.0, dropout_rate=None, weight | |||
| in the transition block. | |||
| dropout_rate: dropout rate | |||
| weight_decay: weight decay factor | |||
| dilation_rate: an integer or tuple/list of 2 integers, specifying the dilation rate to | |||
| use for dilated convolution. Can be a single integer to specify the same value for | |||
| all spatial dimensions. | |||
|
|
|||
There was a problem hiding this comment.
The docstring should also explain pooling parameter. Just copy paste from the DenseNet docstring.
|
|
||
| x_list = [x] | ||
|
|
||
| for i in range(nb_layers): | ||
| x = __conv_block(x, growth_rate, bottleneck, dropout_rate, weight_decay) | ||
| x = __conv_block(x, growth_rate, bottleneck, | ||
| dropout_rate, weight_decay) |
There was a problem hiding this comment.
Should be fit in one line.
There was a problem hiding this comment.
breaks pep8 length limit of 80 characters, required by upstream keras but not yet enabled for keras-contrib (same for other multi-line comments)
There was a problem hiding this comment.
Hmm, it looks absurd. How about putting dropout_rate to the top and weight_decay bottom?
| x1 = concatenate(x_list, axis=concat_axis) | ||
| #x = merge(x_list, mode='concat', concat_axis=concat_axis) | ||
| x = merge(x_list, mode='concat', concat_axis=concat_axis) | ||
| # x = concatenate(x_list, concat_axis) |
There was a problem hiding this comment.
Remove commented line for now. Can be updated once a patch for the bug is merged onto Keras master.
| @@ -552,7 +592,8 @@ def __create_fcn_dense_net(nb_classes, img_input, include_top, nb_dense_block=5, | |||
| assert reduction <= 1.0 and reduction > 0.0, "reduction value must lie between 0.0 and 1.0" | |||
|
|
|||
| # check if upsampling_conv has minimum number of filters | |||
| # minimum is set to 12, as at least 3 color channels are needed for correct upsampling | |||
| # minimum is set to 12, as at least 3 color channels are needed for | |||
| # correct upsampling | |||
| assert nb_upsampling_conv > 12 and nb_upsampling_conv % 4 == 0, "Parameter `upsampling_conv` number of channels must " \ | |||
There was a problem hiding this comment.
I forgot to change this to >= 12. My mistake.
| # not the concatenation of the input with the feature maps | ||
| # (concat_list[0]. | ||
| l = merge(concat_list[1:], mode='concat', concat_axis=concat_axis) | ||
| # l = concatenate(concat_list[1:], axis=concat_axis) |
There was a problem hiding this comment.
Remove for now until the bug is fixed in Keras master
There was a problem hiding this comment.
Will be ready after keras-team/keras#6035 is merged, which is likely quite soon. Also relevant keras-team/keras#6034 and keras-team/keras#5972. I might just wait a day or two instead of removing the comment. :-)
There was a problem hiding this comment.
Thats alright. I would rather keep densenet code compatible with both, but for now I guess my only recourse is to keep a stable copy in my seperate repository so that I can continue using the code with Keras 1.2.2
| # not the concatenation of the input with the feature maps (concat_list[0]. | ||
| l = concatenate(concat_list[1:], axis=concat_axis) | ||
| # not the concatenation of the input with the feature maps | ||
| # (concat_list[0]. |
There was a problem hiding this comment.
"(concat_list[0])." should be after feature maps, not on a separate line
There was a problem hiding this comment.
Made this muli-line for pep8 compliance, I'll improve it a bit in the next commit
|
|
||
| t = __transition_up_block(l, nb_filters=n_filters_keep, type=upsampling_type, output_shape=out_shape) | ||
| t = __transition_up_block( |
There was a problem hiding this comment.
Should be on same line. No reason for new line for a small parameter list.
| x = concatenate([t, skip_list[block_idx]], axis=concat_axis) | ||
| x = merge([t, skip_list[block_idx]], | ||
| mode='concat', concat_axis=concat_axis) | ||
| # x = concatenate([t, skip_list[block_idx]], axis=concat_axis) |
There was a problem hiding this comment.
Remove concatenate code until bug is fixed in Keras master. Also merge call should be one line.
|
@ahundt Has this been trained on the Pascal VOC dataset yet? My laptop is currently training another model ensemble for a class project, and estimated time is another 12-14 hours, so I can't train it myself. As to the changes, they are minor and everything mostly looks good. One thing I am worried about is allowing depth parameter to be None. Could you explain how the code handle depth=None with non atrous pooling? It seems like you are overloading the meaning of depth parameter here. Edit: After looking through the code one more time, it seems you allow depth to be none if nb_dense_block and nb_layers_per_block are set. I suggest adding an assertion such that for the case when that depth=None and nb_layers_per_block is -1 never occurs. A simple assert should do the trick. This will force users to pick one paradigm, either give depth and let nb_layers_per_block be computed from that or keep depth=None and force users to give nb_layers_per_block. |
|
@titu1994 Okay I realized I made a mistake in the transitions and the kernel_size was also needed for transitions if the dilation_rate were to be useful. I've added that plus the activation information, and reparameterized how top is specified, now include_top is specified separately from the kind of top. Hopefully now all is well and good! |
|
@titu1994 I've made changes that integrate the functionality from your pull request, correctly parameterize the densenet transition layers, and should now make densenet.py meet the pep8 requirements so the build will no longer fail soon. Can you review again? |
# Conflicts: # keras_contrib/applications/densenet.py
|
This is very out of date and the densenet + tensorflow memory issue detailed at tensorflow/tensorflow#12948 hobbles this tremendously, so it isn't really practical to use. For those reasons I'm closing this, and if the situation changes a new PR with equivalent functionality can be created. |
Work in progress for the use of atrous convolution to enable segmentation with the standard DenseNet without a U-net structure. By passing
dilation_rate=2andpooling=None, the resulting network will be full resolution with a receptive field equivalent to the original (and still default) version with average pooling. Then the same 2D activation top as DenseNetFCN can be applied for segmentation results. Multi-Scale Context Aggregation by Dilated Convolutions is a good reference on atrous convolutions as is A guide to convolution arithmetic for deep learning and its corresponding conv_arithmetic repository.TODO before merging:
@titu1994 may be interested as the main author of the DenseNet code.