<div dir="ltr"><div><div><div><div><br><br>On Thu, Jan 26, 2017 at 3:18 PM, Markus Metz <<a href="mailto:markus.metz.giswork@gmail.com">markus.metz.giswork@gmail.com</a>> wrote:<br>><br>><br>><br>> On Mon, Jan 23, 2017 at 11:19 AM, Rashad Kanavath <<a href="mailto:mohammedrashadkm@gmail.com">mohammedrashadkm@gmail.com</a>> wrote:<br>> ><br>> ><br>> ><br>> > On Mon, Jan 23, 2017 at 9:22 AM, Moritz Lennert <<a href="mailto:mlennert@club.worldonline.be">mlennert@club.worldonline.be</a>> wrote:<br>> >><br>> >> Hi Rashad,<br>> >><br>> >> On 22/01/17 11:00, Rashad Kanavath wrote:<br>> >>><br>> >>> I fixed all the points from last mail.<br>> >><br>> >><br>> >> Wonderful !<br>> >><br>> >> It seems that you missed one point (see in the ticket #3142/#3247).<br>> >><br>> >> Thank you so much for this great job !<br>> ><br>> ><br>> > Thanks. Sorry that I missed it. I will push a fix for both and update the tracker too.<br>><br>> I found time to review the code and discovered a few more issues:<br>><br>> L210<br>>   pdata  = G_malloc (sizeof (int) * group_ref.nfiles);<br>> must be<br>>   pdata  = G_malloc (sizeof (int *) * group_ref.nfiles);<br><br></div>L301<br>  kseeds  = G_malloc (sizeof (int) * group_ref.nfiles);<br></div>must be<br>  kseeds  = G_malloc (sizeof (double *) * group_ref.nfiles);<br><br></div>L340<br>  sigma  = G_malloc (sizeof (int) * group_ref.nfiles);<br></div>must be<br>  sigma  = G_malloc (sizeof (double *) * group_ref.nfiles);<br><br><div><div><div><div>><br>> L232<br>>     pdata[nf][index] = ((int *) i_band)[col];<br>> works only if the input is CELL_TYPE<br>> i_band should be *DCELL, pdata should be ***DCELL such that L232 becomes<br>>     pdata[row][col][nf] = i_band[col];<br>> the remaining code needs to be updated for changes in pdata<br>><br>> L468<br>> no need to allocate and free a new row buffer for each output row<br>><br>> in SLIC_EnforceLabelConnectivity()<br>> allocating sizeof(int) * nrows * ncols) for both xvec and yvec can cause OOM errors. Superpixels should be much smaller, thus dynamic reallocation would be better.<br>><br>> L586<br>> this is the wrong place to update numlabels, better do<br>> numlabels = SLIC_EnforceLabelConnectivity()<br>> in main() L457<br>><br>> NULL handling must be implemented<br>> cells where any input band is NULL must be skipped.<br>> Initializing seeds must be changed at L323-327 to avoid initializazion with NULLs<br>><br>> A few variables are allocated as vectors even though they hold some info about each cell. They should be allocated as matrices. E.g. klabels is currently accessed as klabels[row * ncols + col], better would be klabels[row][col]. Equivalent for pdata and distvec. This change will make it easier to use the segment library for managing temporary data.<br>><br>> The spatial and spectral distances are currently not comparable, the rather cryptic compactness parameter needs to be used to make them comparable and determine the weight for the spatial and spectral distances. It would be easier to understand if the spatial and spectral distances are separately scaled such that each is in the range [0, 1]. This is already done in i.segment and also described in the SLIC paper in formula 2. Compactness could then be changed to be the weight for spatial distance with default 0.5. Values > 0.5 would favour the spatial distance and cause more compact superpixels while values < 0.5 would favour spectral distance and cause less compact but spectrally more homogeneous superpixels.<br>><br>> The option to perturb seeds could be useful to get more robust initial seeds. A similar mechanism could also be used to find a non-NULL cell as initial seed location.<br>><br>> Currently the module has an option to specify the desired number of superpixels. That makes comparison of the results for the same input but different computational regions difficult. Instead of the number of superpixels, the average superpixel size could be an input option, that would make results comparable.<br>><br>> Markus M<br>><br>> ><br>> ><br>> >><br>> >> Moritz<br>> >><br>> >><br>> >><br>> >>><br>> >>> On Fri, Jan 6, 2017 at 9:28 AM, Markus Metz<br>> >>> <<a href="mailto:markus.metz.giswork@gmail.com">markus.metz.giswork@gmail.com</a> <mailto:<a href="mailto:markus.metz.giswork@gmail.com">markus.metz.giswork@gmail.com</a>>><br>> >>> wrote:<br>> >>><br>> >>><br>> >>><br>> >>>     On Tue, Dec 27, 2016 at 9:05 PM, Rashad Kanavath<br>> >>>     <<a href="mailto:mohammedrashadkm@gmail.com">mohammedrashadkm@gmail.com</a> <mailto:<a href="mailto:mohammedrashadkm@gmail.com">mohammedrashadkm@gmail.com</a>>> wrote:<br>> >>><br>> >>><br>> >>><br>> >>>         I had pushed a updated version of code:<br>> >>><br>> >>>         Done:<br>> >>>         1. input is a imagery group from i.group (handles all raster in<br>> >>>         the group)<br>> >>>         2. removed rgb2lab conversion<br>> >>>         3. implementation of id output map<br>> >>>         4. replaced  fmin with MIN and MAX macro<br>> >>>         5. compactness is a new option to module? (default is 20 from paper)<br>> >>><br>> >>>     Wonderful, thanks!<br>> >>><br>> >>><br>> >>>         TODO:<br>> >>>         1. add option for SLICO<br>> >>>         2. use segment library to avoid memory limitation when reading<br>> >>>         raster data<br>> >>><br>> >>>         Hope this works now. Let me know If I missed something else.<br>> >>><br>> >>>     Code clean-up:<br>> >>>     - please use tools/grass_indent.sh<br>> >>>     - move variable declarations to the beginning of a code block<br>> >>>     - convert C++ style comments to C style comments<br>> >>>     - line 155<br>> >>>       int sz = nrows * ncols;<br>> >>><br>> >>>     can cause integer overflow. Use off_t (large raster maps can only be<br>> >>>     handled with LFS, i.e. off_t being a 64bit integer).<br>> >>><br>> >>>     - fix L157: must be<br>> >>>       int **pdata;<br>> >>><br>> >>>       pdata = G_malloc(sizeof(int) * group_ref.nfiles);<br>> >>><br>> >>>     - Lines 317,318 are correct, line 320 is wrong<br>> >>><br>> >>><br>> >>>         And...<br>> >>><br>> >>>         Happy New Year to all :-)<br>> >>><br>> >>>     Happy new year to you too!<br>> >>><br>> >>>     Markus M<br>> >>><br>> >>><br>> >>><br>> >>>         On Thu, Dec 22, 2016 at 10:12 AM, Markus Neteler<br>> >>>         <<a href="mailto:neteler@osgeo.org">neteler@osgeo.org</a> <mailto:<a href="mailto:neteler@osgeo.org">neteler@osgeo.org</a>>> wrote:<br>> >>><br>> >>>             On Thu, Dec 22, 2016 at 12:01 AM, Markus Metz<br>> >>>             <<a href="mailto:markus.metz.giswork@gmail.com">markus.metz.giswork@gmail.com</a><br>> >>>             <mailto:<a href="mailto:markus.metz.giswork@gmail.com">markus.metz.giswork@gmail.com</a>>> wrote:<br>> >>>             ...<br>> >>>             > Adding<br>> >>>             ><br>> >>>             > #undef MIN<br>> >>>             > #define MIN(a,b) ((a) < (b) ? (a) : (b))<br>> >>>             ><br>> >>>             > as in other GRASS modules is a nice solution to keep things simple and<br>> >>>             > portable.<br>> >>><br>> >>>             ... how about a adding this and similar definitions to gis.h?<br>> >>><br>> >>>             markusN<br>> >>>             _______________________________________________<br>> >>>             grass-dev mailing list<br>> >>>             <a href="mailto:grass-dev@lists.osgeo.org">grass-dev@lists.osgeo.org</a> <mailto:<a href="mailto:grass-dev@lists.osgeo.org">grass-dev@lists.osgeo.org</a>><br>> >>>             <a href="http://lists.osgeo.org/mailman/listinfo/grass-dev">http://lists.osgeo.org/mailman/listinfo/grass-dev</a><br>> >>>             <<a href="http://lists.osgeo.org/mailman/listinfo/grass-dev">http://lists.osgeo.org/mailman/listinfo/grass-dev</a>><br>> >>><br>> >>><br>> >>><br>> >>><br>> >>>         --<br>> >>>         Regards,<br>> >>>            Rashad<br>> >>><br>> >>><br>> >>><br>> >>><br>> >>><br>> >>> --<br>> >>> Regards,<br>> >>>    Rashad<br>> >><br>> >><br>> ><br>> ><br>> ><br>> > --<br>> > Regards,<br>> >    Rashad<br>><br></div></div></div></div></div>