[GRASS-dev] [GRASS GIS] #3287: i.zc: threshold parameter not correctly taken into account

[GRASS GIS]

#3287: i.zc: threshold parameter not correctly taken into account
----------------------------+-------------------------
 Reporter:  mlennert        |      Owner:  grass-dev@…
     Type:  defect          |     Status:  new
 Priority:  normal          |  Milestone:  7.2.1
Component:  Imagery         |    Version:  svn-trunk
 Keywords:  i.zc threshold  |        CPU:  Unspecified
 Platform:  Unspecified     |
----------------------------+-------------------------
 i.zc is an edge detection module. In that module the parameter "threshold"
 is defined as determining "the 'sensitivity' of the Gaussian filter. The
 default value is 10; higher and lower values can be tested by the user.
 Increasing the threshold value will result in fewer edges being found."

 In the code, the value is treated as follows:


 {{{
     sscanf(threshold->answer, "%1lf", &Thresh);
     if (Thresh <= 0.0)
         G_fatal_error(_("Threshold less than or equal to zero not
 allowed"));
     Thresh /= 100.0;
 }}}

 Because of the "%1lf", only the first digit is scanned from the threshold
 answer, making values 10, 100, 15, 1 lead to the same result. Also, one
 cannot go below 1 as 0.5 is read as 0 and 0 is not allowed.

 During tests, I found that a threshold value of 0.01 seems to give
 reasonable default results.

 In a quick fix, I applied the following patch to trunk.


 {{{
 Index: grass/trunk/imagery/i.zc/main.c
 ===================================================================
 --- grass/trunk/imagery/i.zc/main.c     (revision 69783)
 +++ grass/trunk/imagery/i.zc/main.c     (revision 70477)
 @@ -87,5 +87,5 @@
      threshold->multiple = NO;
      threshold->description = _("Sensitivity of Gaussian filter");
 -    threshold->answer = "10";
 +    threshold->answer = "0.01";

      orientations = G_define_option();
 @@ -104,8 +104,7 @@
      inputfd = Rast_open_old(input_map->answer, "");

 -    sscanf(threshold->answer, "%1lf", &Thresh);
 +    Thresh = atof(threshold->answer);
      if (Thresh <= 0.0)
         G_fatal_error(_("Threshold less than or equal to zero not
 allowed"));
 -    Thresh /= 100.0;

      sscanf(width->answer, "%f", &Width);
 }}}

 This changes the way the parameter is read to allow floating point input
 and sets the default to 0.01.

 However, this is a module API change as the default value has been changed
 (before, it was 10/100 = 0.1) and the way to input the value has also
 changed.

 On the other hand, this has never worked before and no one has ever filed
 a bug against this module..

 So, I see the following alternatives (all include the correction to the
 reading of the parameter value):

 * Leave the default as is at 10 aka 0.1
 * Change the default to 1, translated it internally to 0.1 (i.e. the
 current default, just with a different parameter value)
 * Change the default to 1, translated internally to 0.01 (i.e. what I've
 experienced as a more sensible default)
 * Change the default to 0.01 (as currently in trunk)

 Any preferences ? Mine would probably be the third, as I find 1 as default
 value more logical, and I'd rather have the module use a default value
 that gives more interesting results...

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/3287>
GRASS GIS <https://grass.osgeo.org>