I was finally able to review the final commits and merge it in to the 'master' branch. So the task is now being marked as closed :-).

Thanks a lot Natáli,

I really liked your guide on making a polygon region file in DS9, very nice and complete! Generally, your work on the documentation was very impressive :-).

Before getting to the changes I made, let me also mention this: looking at the commit messages, I noticed that in the second one, the title line ended with a period (.)! This is also against our commit conventions (as with many other projects: its just like a title of a section in a book/paper: titles don't end with a period). So I '--amended' your commit and '--force' pushed it to Gitlab in Commit 2c3731114. So before continuing the development of this branch, pull this new commit and work on top of this ;-).

You had also mentioned that this message is a re-write of a previous one. Don't forget that the previous one is no longer in the history and nothing but the format has changed, so this will only confuse the future readers of the history ;-). So I removed it in the edit.

After building and testing over the commits, I made some improvements in Commit 36e8b53 (commit message shown in P.S.). Please study the changes carefully to help smooth your future commits for merging :-).

Generally, I feel this is fine to be merged into 'master'. The only remaining thing is your copyright statement. Please stay in touch with the GNU Copyright clerk (and remind them if necessary), so that can be done as soon as possible.

One possible/optional improvement that occurs to me is this (to help use the time until the copyright is assigned): since file names are very different from numbers, we can remove the new '--polygonfile' overall and let the user specify the file directly with '--polygon':

--polygon=1.234,5.678:1.234,5.678:1.234,5.678
--polygon=filename

When the user gives something like '--polygon=filename', instead of parsing it as a series of numbers with 'gal_options_parse_colon_sep_csv_raw', we can immediately give the string to 'gal_ds9_reg_read_polygon'.

To do this, a small check has to be added in 'gal_options_parse_colon_sep_csv' (in 'lib/options.c'). For example if a comma (,) and colon (:) exist in the string, we can interpret the value to '--polygon' as a series of numbers. Otherwise, we can interpret the value to '--polygon' as a filename.

The coordinate mode can be written into the 'status' component of the output 'gal_data_t' afterwards (it was precisely defined for such situations).

This would also be much more user-friendly ;-).

P.S. Message of Commit 36e8b53:

    Crop and Table: --polygonfile new name for --polygonname

    Until now, the '--polygonname' option was used to specify the SAO DS9
    region file that contains the polygon to use. However, this could be
    confused with the actual name of some polygons that have a name: for
    example "triangle" or "octagon"!

    With this commit, the option has been renamed to '--polygonfile' in both
    Crop and Table. Also, I noticed that the short '-t' option was only used
    for Crop and not Table. Generally, when there is a similar option between
    two programs, they should behave similarly to avoid confusion for the
    users. So the short format of this option was removed in Crop is well.

    In the process, some other changes have been made which are listed below:

     - The NEWS file has been updated with all the new features added.

     - In Table, the coordinate system of the vertices is irrelevant: the user
       specifies the columns to check with and there is no way we can know if
       they are image or WCS. So the check on having a reasonable coordinate
       has been removed. But generally, Natáli, when you want to break a
       conditional into two lines, bring the conditional operator to the start
       of the next/second line, not the end of the first line. This helps a lot
       in readability (the eye is much less prone to miss something at the
       start of a line, rather than at the end of it), like below:

          if( ds9regmode!=GAL_DS9_COORD_MODE_IMG
              && ds9regmode!=GAL_DS9_COORD_MODE_WCS )

     - In the book, I generally noticed that sometimes "SAO ds9" was used and
       some times "SAO DS9" (long before this branch). So to unify things, they
       are all now "SAO DS9" (following its own webpage).

     - In the book, some index items were added (with '@cindex').

     - In the book, the expected format of the region file has been taken to
       the 'gal_ds9_reg_read_polygon' function. In the description of
       '--polygonfile', it only points the interested reader to there. This
       helps most users of Crop or Table (a high-level user will generally not
       be interested in low-level things and can even find them annoying and
       distracting).

     - In the book, the polygon line of the example region file was too long
       for the printed format (created with 'make pdf'). So I shortened the
       example vertices.

     - In the book, at the start of the library section on SAO DS9, there was
       no description of what SAO DS9 actually is. I know its clear for most
       astronomers, but the library is sometimes inspected by non-astronomers,
       so its always good to start each section with the most basic description
       and explain a little more on what the whole purpose is for someone with
       no background.

     - In the book, the three macros were brought into one '@deffn' (second and
       third with a '@deffnx') Since they are related, this helps in
       readability and avoids the need to repeat things in the description.

Thanks Natáli!

I pulled the commits and had a look, the changes look good and but when I saw the commit messages, I couldn't continue! In terms of content they are good, the problem is in the length of each line.

Please run 'git log' and compare your commit messages to the previous commit messages ;-). You will see that no commit message line is longer than 75 characters. This is intentional because we want the output of 'git log' to be readable in a standard 80-character terminal window.

Can you please re-format the commit messages to follow the same convention?

Recall that these development commits will later go into Gnuastro's history, so we need to have them in a nice and common format (the history of the project is also very important).

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 (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, 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 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:

    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.

Allow users to use a file for polygon vertices instead of having to type all the coordinates in by hand. Typing coordinates (especially for larger number of vertices) can be prone to errors and is frustrating.

However, in GUI tools like SAO DS9, we can easily define a polygon region on the image and save the vertices into a file with specific formats. So when the input is a file name (not a string of numbers), Crop can parse the output of DS9 directly and users don't have to deal with the actual numbers. This will greatly speed up the process and reduces chances of error.

Until then, a script like this can be used to read the DS9 region file (saved in the `ds9' format in `fk5', see P.S. below) and prepare it for Crop (new name for ImageCrop) as a shell variable):

$ v=$(awk 'NR==4' ds9.reg | sed -e's/polygon(//'        \
           -e's/\([^,]*,[^,]*\),/\1:/g' -e's/)//' )
$ astimgcrop --mode=wcs image.fits --polygon=$v

This task was suggested by Guillaume Mahler.


---
P.S.

To define a region file in SAO DS9, you can take the following steps: Open the image with DS9 and activate its "Region" mode with "Edit --> click --> Region". Then define the region to be a polygon with "Region --> Shape --> Polygon". Click on the approximate center of the region you want and a small square will appear. By clicking on the vertices of the square you can shrink or expand it, clicking and dragging anywhere on the edges will enable you to define a new vertice.

After the region has been nicely defined, save it as a file with "Region --> Save" Regions. You can then select the name and address of the output file, keep the format as "REG" and press "OK". In the next window, keep format as "ds9" and "Coordinate System" as "fk5". A plain text file (let's call it `ds9.reg') is now created that you can feed into the script above.