i.zc: threshold parameter not correctly taken into account

[mlennert]

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...

[annakrat]

I am not sure I understand entirely all the options, but I think you should decide based on your understanding. Third options sounds reasonable.

[mlennert]

I set the default parameter value to 1, internally divided by 100, in both trunk (r70688) and rel72 (r70689).

Closing the ticket.