[gnuastro-commits] master 326b42a 2/9: Crop: polished the implementation of new --polygonname option

[Mohammad Akhlaghi]

branch: master
commit 326b42aeaf0da5fcb5d4c2ef254bd9cb37e7b1a0
Author: Mohammad Akhlaghi <mohammad@akhlaghi.org>
Commit: Mohammad Akhlaghi <mohammad@akhlaghi.org>

    Crop: polished the implementation of new --polygonname option
    
    In the previous commit Natáli implemented a first implementation of this
    function which looked very good. But some points needed to be corrected to
    polish this function for both optimised processing and cleaner and more
    consistent code:
    
     - The commit message of the previous commit was a single line with no
       classification (specifying what program/library it relates to) and no
       description of the job. As you see from a simple 'git log', commit
       message descriptions (WHY something was done, the HOW can be ommitted)
       are very important in Gnuastro.
    
     - Some lines ended with an empty character (space or TAB). Git shows these
       lines at red regions. So its always good to inspect the committed
       changes with 'git diff' and remove such lines if you find them.
    
     - The step to test for the new option was in the very high-level
       'ui_read_check_inputs_setup' function. This function is by nature VERY
       high-level, and to help in modularity and readability, it shouldn't
       contain any type of low-level processing. The necessary checks and
       processing should be done in one of the functions that are called within
       it. In the case of this check, the best place was this function:
       'ui_read_check_only_options'. As the name says, this function is
       designed to "check only options". And since this new options operation
       is pretty fundamental (it can change the '--mode', see below), this
       check is done right at the start.
    
     - While this function is only used in this '.c' file, it didn't have a
       'static' prefix before it. It helps in human and computer readability
       and debuggin to add the 'static' prefix when a function is intended only
       for one file. Functions that don't have a 'static' behind them should be
       in the '.h' file of that '.c' file (because they can also be called from
       other '.c' files).
    
     - The output type of the function ('void') was on the same line as the
       function name. If you look at all other Gnuastro functions you will
       notice that the type should always be on the previous line. This is a
       GNU convention and again, greatly helps in readability ;-).
    
     - Since the function was moved to a new place, I renamed it to
       'ui_polygon_from_ds9_reg'.
    
     - The variable definitions at the start of the functions were not sorted
       by length. As discussed in the Gnuastro coding conventions, it greatly
       helps in readability when grouped lines that are independent of each
       other are sorted by length.
    
     - There was no check to confirm if the opened file is indeed a DS9 region
       file (users often mistakenly give file names, so the nature of a file
       should always be checked to see if it is what the program expects or
       not). I added a step to see if the first line of the file has the string
       '# Region file format: DS9'.
    
     - When you want to make sure that something is not 0 or not NULL, instead
       of writing 'if(a!=NULL)' or 'if(a!=0)', just use 'if(a)'. This helps in
       readability and also removes the potential for many hard-to-find bugs.
    
     - Generally, try to avoid any '!=NULL' checks when its possible. For
       example, see my new implementation for the check of this option in
       'ui_read_check_only_options'. To avoid the check, I simply brought the
       error message (when '--polygon' and '--polygonname' are given together)
       on top.
    
     - While parsing the file, certain line-numbers were assumed for certain
       features. For example it was assumed that the third line contains the
       coordinate type and the 4th line contained the polygon. These types of
       assumptions greatly reduce the portability of your code. For example
       someone may want to manually put comment lines somewhere in the file, or
       certain ds9 settings (that are printed in the second line) may not
       actually be printed in some DS9s (based on user configuration). So as
       you see, I used the first characters of the line to check for the
       desired feature.
    
     - For the coordinate mode, the old function was checking with the
       system's. But generally, the DS9's coordiante mode corresponds to the
       mode of the numbers. So when a user gives a DS9 region file, they don't
       have to worry about the coordinate mode any more ;-)! So in the new
       implementation, I just set Crop's internal mode based on the DS9 file
       (and over-write any user-given '--mode'). We should later add this in
       the description of this option.
    
     - To parse the polygon vertice coordinates, I used an internal function in
       Gnuastro that is now called 'gal_options_parse_colon_sep_csv_raw' This
       is the same core function that parses the values given to
       '--polygon'. The only tricky part was that this function expects each
       coordinate pair to be separated by a ':'. So before giving the string to
       this function, I simply parsed it and converted every second ',' to a
       ':'. Since the core function is the same, the outputs will also be the
       same and everything works fine afterwards. Also the code for the parsing
       function becomes simpler and easier to read/debug.
    
     - Generally, once you have all the information you want, there is no need
       to continue parsing the file. So as you see, in the new function, after
       reading the
---
 bin/crop/ui.c                   | 288 ++++++++++++++++++++--------------------
 lib/gnuastro-internal/options.h |   4 +
 lib/options.c                   |   7 +-
 3 files changed, 150 insertions(+), 149 deletions(-)

diff --git a/bin/crop/ui.c b/bin/crop/ui.c
index 913e94c..f6f6fa8 100644
--- a/bin/crop/ui.c
+++ b/bin/crop/ui.c
@@ -269,6 +269,125 @@ ui_parse_coordinate_mode(struct argp_option *option, char 
*arg,
 /**************************************************************/
 /***************       Sanity Check         *******************/
 /**************************************************************/
+static void
+ui_polygon_from_ds9_reg(struct cropparams *p)
+{
+  FILE *fp;
+  char *polygonstr;
+  size_t commacounter=0;
+  int coordmode=IMGCROP_MODE_INVALID;
+  size_t plinelen, linesize=10, lineno=0;
+  char *c, *line, *ds9regstart="# Region file format: DS9";
+  char *polygonformaterr="It is expected for the line to have "
+    "this format: 'polygon(AAA,BBB,...)'. Where 'AAA' and 'BBB' "
+    "are numbers and '...' signifies that any number of points "
+    "are possible";
+
+  /* Allocate size to the lines on the file and check if it was sucessfull.
+     The getline function reallocs the necessary memory. */
+  errno=0;
+  line = malloc(linesize * sizeof(*line));
+  if(line == NULL)
+    error(EXIT_FAILURE, errno, "%s: %zu bytes for line buffer",
+          __func__, linesize * sizeof(*line));
+
+  /* Open the file and checks if it's not null. */
+  errno=0;
+  fp = fopen(p->polygonname, "r");
+  if(fp == NULL)
+    error(EXIT_FAILURE, errno, "The polygon file is blank");
+
+  /* Get the lines on the file. */
+  while(getline(&line, &linesize, fp)!=-1)
+    {
+      /* To have line-counters starting from 1. */
+      ++lineno;
+
+      /* The first line should start with a fixed string. */
+      if(lineno==1)
+        {
+          if( strncmp(line, ds9regstart, 25) )
+            error(EXIT_FAILURE, 0, "%s: doesn't appear to be a DS9 "
+                  "region file! We assume that DS9 region files begin "
+                  "with this string in their first line: '%s'",
+                  p->polygonname, ds9regstart);
+          continue;
+        }
+
+      /* If we are on the coordinate mode line, then set the mode of Crop
+         based on it. */
+      if( !strcmp(line, "fk5\n") || !strcmp(line, "image\n") )
+        {
+          /* Make sure it hasn't been called more than once. */
+          if(coordmode!=IMGCROP_MODE_INVALID)
+            error_at_line(EXIT_FAILURE, 0, p->polygonname, lineno,
+                          "more than one coordinate line defined");
+
+          /* Set the proper mode. */
+          if(!strcmp(line, "fk5\n")) coordmode=IMGCROP_MODE_WCS;
+          else                       coordmode=IMGCROP_MODE_IMG;
+
+          /* Stop parsing the file if the polygon has also been found by
+             this point (we don't need any more information, no need to
+             waste the user's CPU and time). */
+          if(p->polygon) break;
+        }
+
+      /* The line containing the polygon information starts with
+         'polygon('. */
+      if( !strncmp(line, "polygon(", 8) )
+        {
+          /* Get the line length and check if it is indeed in the proper
+             format. If so, remove the last parenthesis. */
+          plinelen=strlen(line);
+          if(line[plinelen-2]==')')
+            line[plinelen-2]='\0';
+          else
+            error_at_line(EXIT_FAILURE, 0, p->polygonname, lineno,
+                          "line with polygon vertices doesn't end "
+                          "with ')'. %s", polygonformaterr);
+
+          /* Convert the string to the expected format (with ':' separating
+             each vertice). Note how we are ignoring the first 8 characters
+             that contain 'polygon('. */
+          polygonstr=&line[8];
+          for(c=polygonstr; *c!='\0'; ++c)
+            if(*c==',' && (++commacounter % 2) == 0 ) *c=':';
+
+          /* Read the coordinates within the line. */
+          p->polygon=gal_options_parse_colon_sep_csv_raw(polygonstr,
+                                                         p->polygonname,
+                                                         lineno);
+
+          /* Stop parsing the file if the coordinate mode has also been
+             found by this point (we don't need any more information, no
+             need to waste the user's CPU and time). */
+          if(coordmode!=IMGCROP_MODE_INVALID) break;
+        }
+    }
+
+  /* If no coordinate mode was found in the file, print an error. */
+  if(coordmode==IMGCROP_MODE_INVALID)
+    error(EXIT_FAILURE, 0, "%s: no coordinate mode found! "
+          "We expect one line to be either 'fk5' or 'image'",
+          p->polygonname);
+
+  /* If no polygon line was in the file, abort with an error. */
+  if(p->polygon==NULL)
+    error(EXIT_FAILURE, 0, "%s: no polygon statement found! We expect "
+          "one line in the format of 'polygon(AAA,BBB,...)' in the "
+          "file given to '--polygonname' option. %s", p->polygonname,
+          polygonformaterr);
+
+  /* Free the space used. */
+  free(line);
+  fclose(fp);
+}
+
+
+
+
+
 /* Read and check ONLY the options. When arguments are involved, do the
    check in 'ui_check_options_and_arguments'. */
 static void
@@ -277,6 +396,26 @@ ui_read_check_only_options(struct cropparams *p)
   double *darray;
   int i, checksum;
 
+  /* If a DS9 region file should be used for the polygon, read it. This is
+     done first for two reasons. 1) It can change the value of '--mode'. 2)
+     It will set the '--polygon' option's value. */
+  if(p->polygonname)
+    {
+      if(p->polygon)
+        error(EXIT_FAILURE, errno, "'--polygon' and '--polygonname' "
+              "cannot be given together. With the first you specify the "
+              "polygon vertices directly on the command-line. With the "
+              "second, you give a DS9 region file and the polygon "
+              "vertices are read from that.");
+      else
+        {
+          ui_polygon_from_ds9_reg(p);
+          free(p->polygonname);
+          p->polygonname=NULL;
+        }
+    }
+
+
   /* Make sure that only one of the crop definitions is given. */
   checksum = ( (p->center!=NULL)
                + (p->catname!=NULL)
@@ -983,6 +1122,8 @@ ui_preparations_to_img_mode_values(struct cropparams *p)
 
 
 
+
+
 void
 ui_preparations(struct cropparams *p)
 {
@@ -1147,139 +1288,6 @@ ui_preparations(struct cropparams *p)
 
 
 /**************************************************************/
-/*********             Parser ds9 file             ************/
-/**************************************************************/
-size_t ui_ds9_coordinates_number(char *line)
-{
-  char *token;
-  char delimiters[] = "(,)\n";
-  size_t quantity = 0;
-  char *var;
-  
-  /* Copy the line to another variable to inspect it. */
-  var = malloc ((strlen(line) + 1) * sizeof(*var));
-  strcpy(var, line);
-  
-  /* Get the first token (the geometry of the region). */
-  token = strtok(var, delimiters);
-  
-  /* The rest of the tokens are coordinates. So go through them and count. */
-  while(token != NULL)
-  {
-    ++quantity;
-    token = strtok(NULL, delimiters);
-  }
-  
-  free(var);  
-  
-  /* Check if quantity is zero to avoid returning -1. */
-  if(quantity==0)
-    ++quantity;
-  return(quantity-1);
-}
-
-
-void ui_read_ds9_file(struct cropparams *p)
-{
-  FILE *fp;
-  size_t linesize, lineno, coordno;
-  char *line, *token, *coordmode, delimiters[] = "(,)\n";
-  int modecmp, modecmp2, polygoncmp, coordinatesit;
-  double *coordinates = NULL;
-  struct gal_data_t *newpolygon;
-    
-  /* Allocate size to the lines on the file and check if it was sucessfull.
-     The getline function reallocs the necessary memory. */
-  linesize=10;
-  line = malloc(linesize * sizeof(*line));
-  errno=0;
-  if(line == NULL)
-    error(EXIT_FAILURE, errno, "Given polygon file and coordinates! Choose" 
-          "one.");
- 
-  /* Open the file and checks if it's not null. */
-  fp = fopen(p->polygonname, "r");
-  if(fp == NULL)
-    error(EXIT_FAILURE, errno, "The polygon file is blank.");
- 
-  /* Get the lines on the file. */
-  lineno=0;
-  while(getline(&line, &linesize, fp)!=-1)
-    {
-      ++lineno;  
-      /* Line 3 has the type of the coordinates. */
-      if(lineno==3)
-        {
-          /* Check if the mode on the file is consistent with the mode
-             selected to crop. */
-          coordmode = strtok(line, "\n");
-          modecmp = strcmp(coordmode, "fk5");
-          modecmp2 = strcmp(coordmode, "image");
-          if((!modecmp && p->mode!=IMGCROP_MODE_WCS) || 
-             (!modecmp2 && p->mode!=IMGCROP_MODE_IMG))
-              error(EXIT_FAILURE, errno, "The program coordinates mode is" 
-              " different from the file mode.\nThe mode in the file is: %s",
-              coordmode);
-        }
-      /* Line 4 has the geometry and the coordinates. */ 
-      if(lineno == 4)
-        {
-          /* Gets the number of coordinates given. */
-          coordno = ui_ds9_coordinates_number(line);
-          /* Checks if its a polygon and if the number of coordinates is 
-             valid (divisible by two, because we are dealing with 2d, and if
-             its greater than zero. */
-          token = strtok(line, delimiters);
-          polygoncmp = strcmp(token, "polygon");
-          if(polygoncmp==0 && coordno>0 && coordno%2==0)
-            {
-              /* Allocate size to the coordinates and set it. */
-              coordinates = malloc ((coordno + 1) * sizeof(*coordinates));
-              token = strtok(NULL, delimiters);
-              *coordinates = atof(token);
-              token = strtok(NULL, delimiters);
-              coordinatesit=1;
-              while(token != NULL)
-                {
-                    *(coordinates + coordinatesit) = atof(token);
-                    ++coordinatesit;
-                    token = strtok(NULL, delimiters);
-                }
-              }
-          else
-            error(EXIT_FAILURE, errno, "It's not a polygon or the number"
-                  " of coordinates is invalid: %ld found.",coordno);
-        }
-    }
-    /* Create the polygon structure and points the main parameter to it. */
-    newpolygon = gal_data_alloc(coordinates, GAL_TYPE_FLOAT64, 1, &coordno,
-                                NULL, 0, -1, 1, NULL, NULL, NULL);
-    p->polygon = newpolygon;
-    /* Free the space used. */
-    free(line);
-    fclose(fp);
-}
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-/**************************************************************/
 /************         Set the parameters          *************/
 /**************************************************************/
 
@@ -1310,19 +1318,7 @@ ui_read_check_inputs_setup(int argc, char *argv[], 
struct cropparams *p)
   errno=0;
   if(argp_parse(&thisargp, argc, argv, 0, 0, p))
     error(EXIT_FAILURE, errno, "parsing arguments");
-  
-  
-  /* If it's a polygon file from ds9, we adjust the polygon parameter to 
-     hold the values in the file. */
-  if(p->polygonname!=NULL)
-    {
-      if(p->polygon == NULL)
-          ui_read_ds9_file(p);
-      else
-          error(EXIT_FAILURE, errno, "Given polygon file and coordinates!"
-                " Choose one.");
-    }
-  
+
 
   /* Read the configuration files and set the common values. */
   gal_options_read_config_set(&p->cp);
diff --git a/lib/gnuastro-internal/options.h b/lib/gnuastro-internal/options.h
index 343ebf8..39f2b06 100644
--- a/lib/gnuastro-internal/options.h
+++ b/lib/gnuastro-internal/options.h
@@ -324,6 +324,10 @@ void *
 gal_options_parse_name_and_float64s(struct argp_option *option, char *arg,
                                     char *filename, size_t lineno, void *junk);
 
+gal_data_t *
+gal_options_parse_colon_sep_csv_raw(char *instring, char *filename,
+                                    size_t lineno);
+
 void *
 gal_options_parse_colon_sep_csv(struct argp_option *option, char *arg,
                                 char *filename, size_t lineno, void *junk);
diff --git a/lib/options.c b/lib/options.c
index dc06359..2dbbb04 100644
--- a/lib/options.c
+++ b/lib/options.c
@@ -1387,8 +1387,9 @@ gal_options_parse_name_and_float64s(struct argp_option 
*option, char *arg,
 /* Parse strings like this: 'num1,num2:num3,n4:num5,num6' and return it as
    a data container array: all elements are simply placed after each other
    in the array. */
-static gal_data_t *
-options_parse_colon_sep_csv(char *instring, char *filename, size_t lineno)
+gal_data_t *
+gal_options_parse_colon_sep_csv_raw(char *instring, char *filename,
+                                    size_t lineno)
 {
   char *tailptr;
   gal_data_t *out;
@@ -1529,7 +1530,7 @@ gal_options_parse_colon_sep_csv(struct argp_option 
*option, char *arg,
                       "given to '--%s'", option->name);
 
       /* Parse the desired format and put it in this option's pointer. */
-      dataset=options_parse_colon_sep_csv(arg, filename, lineno);
+      dataset=gal_options_parse_colon_sep_csv_raw(arg, filename, lineno);
 
       /* Add the given dataset to the end of an existing dataset. */
       existing = *(gal_data_t **)(option->value);