[task #14409] Polygon vertices from a DS9 region file

[Mohammad Akhlaghi]

Update of task #14409 (project gnuastro):

                Category:                    Crop => Libraries              
        Percent Complete:                      0% => 90%                    

    _______________________________________________________

Follow-up Comment #2:

This will be a VERY USEFUL feature in Crop, thanks a lot Natáli!

I also ran it and it worked perfectly! Nice job ;-).

However, the function could benefit from some polishing and as I went through
the code, I also noticed some important points that are now committed over
your commit in Commit 6d5db432a79
<https://gitlab.com/makhlaghi/gnuastro-dev/-/commit/6d5db432a79> (commit
message shown below in P.S.)

Almost all of the points are natural for a first time contribution to a
project, so don't worry about the number of points ;-). So I tried to
elaborate/explain everything a lot for you so hopefully they are not repeated
in the future.

In terms of Crop, the work is almost done now. But just to get your hands a
little more dirty into the code, let me make an easy proposal: the Table
program also has a '--polygon' option that reads polygon vertices and extracts
rows that are within/outside the polygon. So this function can also be used
there. To generalize the process, in Commit 5d47ed443ed27
<https://gitlab.com/makhlaghi/gnuastro-dev/-/commit/5d47ed443ed27>, I moved
this function into a new Gnuastro library file called 'lib/ds9.c' and
corrected its calling in 'bin/crop/ui.c'. I also generalized this task to the
"Libraries".

So the remaining steps to get this task done are the following (very easy):

* Implement this feature in the Table program too.
* Add the documentation of the new option into the book (the seconds on Crop
and Table). 
* Add the documentation of the new function in the book (the Libraries).

If you are applying for GSoC 2021, you can do these after the application in
case you don't have time ;-).

P.S. Commit 6d5db432a79
<https://gitlab.com/makhlaghi/gnuastro-dev/-/commit/6d5db432a79> message:


    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


P.Ss: Message of Commit 5d47ed443ed27
<https://gitlab.com/makhlaghi/gnuastro-dev/-/commit/5d47ed443ed27>:


    Library: new ds9.h library functions for parsing ds9 files
    
    Until now, there was only a single 'ui_polygon_from_ds9_reg' function in
    the Crop program for reading a DS9 region file. However, other programs
    (like Table) also have this feature (and would thus need this
    function). Also, generally, there are more operations that can be done on
    DS9 region files or other types of ds9 output files.
    
    With this commit, that function has been generalized into a new library
    file called ('lib/ds9.c', with its corresponding 'lib/gnuastro/ds9.h'
    header). Its calling with the crop program has also been correspondingly
    corrected.


    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/task/?14409>

_______________________________________________
  Message sent via Savannah
  https://savannah.gnu.org/