[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[task #14409] Polygon vertices from a DS9 region file
From: |
Mohammad Akhlaghi |
Subject: |
[task #14409] Polygon vertices from a DS9 region file |
Date: |
Sat, 10 Apr 2021 18:04:04 -0400 (EDT) |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:87.0) Gecko/20100101 Firefox/87.0 |
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/