I have addressed the issues and pushed the fixed commit.

I apologise for these issues. This commit message is fixed now.

I had mistakenly not committed those changes earlier. It should be done now.

I have made the changes and pushed them to github. What guidelines should I follow for the commit message?

I have pushed the commits adding operators to convert decimal to declination and right ascension as well.

I just saw the vice versa thing. We need to convert degrees to RA and declination as well. I'll write the necessary functions for it.

I tried various operators to find the issue. This issue is not caused by the units operator. The memory leak is evident as follows this was a simple -c"arith c1 c1 +" argument.

The table was as follows
1
2

==14516== HEAP SUMMARY:
==14516==     in use at exit: 154 bytes in 5 blocks
==14516==   total heap usage: 1,372 allocs, 1,367 frees, 127,448 bytes allocated
==14516==
==14516== 2 bytes in 1 blocks are definitely lost in loss record 1 of 3
==14516==    at 0x483A7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==14516==    by 0x33F32C: gal_checkset_allocate_copy (checkset.c:154)
==14516==    by 0x4C0ED4: gal_type_from_string (type.c:492)
==14516==    by 0x361244: gal_options_read_check (options.c:1520)
==14516==    by 0x361A5F: options_set_from_name (options.c:1857)
==14516==    by 0x361CD1: options_parse_file (options.c:1949)
==14516==    by 0x361F09: gal_options_parse_config_files (options.c:2034)
==14516==    by 0x36233F: gal_options_read_config_set (options.c:2150)
==14516==    by 0x11092F: ui_read_check_inputs_setup (ui.c:1104)
==14516==    by 0x10D61D: main (main.c:46)
==14516==
==14516== 152 (32 direct, 120 indirect) bytes in 1 blocks are definitely lost in loss record 3 of 3
==14516==    at 0x483A7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==14516==    by 0x10DD23: ui_outcols_add_new_to_end (ui.c:426)
==14516==    by 0x10E051: ui_columns_prepare (ui.c:567)
==14516==    by 0x10F2FB: ui_preparations (ui.c:973)
==14516==    by 0x11096B: ui_read_check_inputs_setup (ui.c:1125)
==14516==    by 0x10D61D: main (main.c:46)
==14516==
==14516== LEAK SUMMARY:
==14516==    definitely lost: 34 bytes in 2 blocks
==14516==    indirectly lost: 120 bytes in 3 blocks
==14516==      possibly lost: 0 bytes in 0 blocks
==14516==    still reachable: 0 bytes in 0 blocks
==14516==         suppressed: 0 bytes in 0 blocks
==14516==

I suspect this issue is caused because the memory allocated to output columns is not freed after displaying the output. I don't know to fix this. Should I file a new bug ?

Great! Then try your best to find the problem, it may be a good bug that you can fix (and add to your application) ;-).

By the way, did the copyright forms go through?

On the general subject of a wrapper library, we are already having some discussions in task #13786. Mosè has some very good experience with Julia and I am sure he can help you a lot in the Julia part (maybe we can be your co-mentors if you get accepted). I'll add you both to the participants of task #13786.

By the way, when you put code output on Savannah, it is easier to read if you put it in the correct quote format ;-). You can see Savannah's simple Markup guidelines. This link is always present while you are writing, its located right above the comment box: the "Rich Markup" link that is just to the left of the "Preview" button ;-).

I'd love to help, but I am just too busy these days unfortunately and given that you are just starting with Gnuastro, I think its a good exercise to master it :-). Try to fix it your self, I'll try it out as soon as I find some free time sometime in the next days.

But I am sure you can find it yourself, just look into the functions and line numbers mentioned by Valgrind.

But maybe you can also focus on your proposal now, and mention that you are already working on the code and this task in particular ;-).

I got your branch and tried it out, but many corrections are still necessary! For example Valgrind still reports bad memory usage:

==32624== Invalid read of size 1
==32624==    at 0x4BFEDD: gal_units_extract_decimal (units.c:22)
==32624==    by 0x4BFF81: gal_units_ra_to_decimal (units.c:68)
==32624==    by 0x126DF3: arithmetic_unary_function (arithmetic.c:496)
==32624==    by 0x1465C7: gal_arithmetic (arithmetic.c:2014)
==32624==    by 0x11123B: arithmetic_operator_run (arithmetic.c:595)
==32624==    by 0x111338: arithmetic_reverse_polish (arithmetic.c:641)
==32624==    by 0x111548: arithmetic_operate (arithmetic.c:728)
==32624==    by 0x1126FB: table (table.c:542)
==32624==    by 0x10CBE8: main (main.c:49)
==32624==  Address 0x5ed0529 is 0 bytes after a block of size 9 alloc'd
==32624==    at 0x483977F: malloc (vg_replace_malloc.c:309)
==32624==    by 0x550ABCE: strdup (in /usr/lib/libc-2.31.so)
==32624==    by 0x4BFDB2: gal_units_extract_decimal (units.c:20)
==32624==    by 0x4BFF81: gal_units_ra_to_decimal (units.c:68)
==32624==    by 0x126DF3: arithmetic_unary_function (arithmetic.c:496)
==32624==    by 0x1465C7: gal_arithmetic (arithmetic.c:2014)
==32624==    by 0x11123B: arithmetic_operator_run (arithmetic.c:595)
==32624==    by 0x111338: arithmetic_reverse_polish (arithmetic.c:641)
==32624==    by 0x111548: arithmetic_operate (arithmetic.c:728)
==32624==    by 0x1126FB: table (table.c:542)
==32624==    by 0x10CBE8: main (main.c:49)
==32624==

Generally, the code still needs a lot of work. Here is a fast list:

• There is no copyright on units.c.
• Don't use `unsigned int', when something relates to sizes (like the `i' or `n' variables, use `size_t'.
• The delimiter given to `strtok' must be a string (ending in `\0'). It can't be a pointer to a character! If it doesn't see a `\0' at the end of the delimiter, `strtok' will continue reading into un-allocated memory.
• When you use the `error' function, there is no more need for a `return 0', the full program will crash with the given warning. If you don't want it to crash, you should just give a `0' for the first argument.

Please try it further (and be sure to run it with Valgrind) and fix these problems, then let me know ;-).

You can put some test `printf' statements in various places during the process to see where this is happening ;-).

Just be careful, you don't want to use `astarithmetic' (that is only for images). You want to use `asttable' with its column arithmetic features.

Please try to debug it yourself, its a good exercise ;-).

Tip: When it says "Conditional jump or move depends on uninitialised value", it means that you are going beyond the array (string in this case) into un-allocated memory.

Use Valgrind ;-)! Its your best friend when dealing with segmentation faults :-)!

Are you using the standard `./developer-build -d' to build Gnuastro and `./tests/during-dev.sh' to execute the test command? If so, you can just add `valgrind' at the start of the third-last line that executes the built utility.

This is because Gnuastro's build system doesn't yet know that it should also compile your new library ;-)!

To tell it to also compile your new `units.c', add `units.c' to the `libgnuastro_la_SOURCES' variable of `lib/Makefile.am'. Just note that the files in this variable are sorted alphabetically (for human-reasons to find easily when reading!), so add it in the correct place.

You also need to tell it to package and install the new header file. So also add `$(headersdir)/units.h' to `pkginclude_HEADERS'.

Afterwards run `autoreconf -f' in the top source directory (so all the necessary build infrastructure is updated), and re-configure and re-built. It should then work ;-)

Some notes:

• The operator is currently called `units_ra_decimal', but this is a little too long for easy typing on the command-line and its not too clear. I recommend setting it to `ra-to-decimal'.
• The operator macro is defined in `arithmetic.h' but is called `GAL_UNITS_CONVERT_RA_DECIMAL'! As you see from all the other operator macros, it should start with `GAL_ARITHMETIC' (because it is located in `arithmetic.h'). Also, don't forget to correct the new operator name in the macro name too ;-).
• This operator is generally placed after the `log10' operator, immediately under it. It is indeed a unary operator, so you can keep it under `log10' in `gal_arithmetic'. But this is a new type of operator: its not like `log' or `sqrt', it takes a string. So I would recommend that in other places group it separately (put an empty line before and after it, with a comment). Later, you also want to put the Dec conversion operator in the same group.
• For the Copyright of `units.c' and `units.h', please put yourself under "Original author", and put me under "Contributing author" ;-).
• For now, probably its better to set `gal_units_extract_decimal'  as a static library of `units.c' and remove it from `units.h'. Later, we should move it some other string-related library.
• For the name of the libraries, generally the Units library will be about conversion. So setting the names like this will be  shorter and more clear to the point: `gal_units_ra_to_decimal' and `gal_units_dec_to_decimal'.
• Remove the `main' function from `units.c'!!!!

In the next comment, please also call your new operator in the Table program, and actually try it on a table. You can then send your test table with the comment here, so I can just download your table, fetch your git commits and try it ;-).

I noticed that you have pushed your commits over the `master' branch!!! As I had described in the forking tutorial you shouldn't do this! Please read it again and follow the steps properly ;-).

No problem about the name, only the repository URL is important for me ;-).

It is easier for me to check the changes as a Git branch. Can you please fork Gnuastro, apply the changes on a new branch, push it to your own repository and give me the link? You can see the process in the Gnuastro forking tutorial.

I think as you said the code will go in a new file. I think the code will be similar to that in lib/arithmetic.c . I am adding the code in it for now so that we can proceed ahead while we wait for the copyright procedure to be completed.

Great! This is perfect! Just let me know when the copyright forms are complete ;-).

The code is very good now, good job ;-).

Just some small points:

1. I still don't see any comments in the two high-level functions `gal_units_convert_ra_decimal' and `gal_units_convert_dec_decimal'. Ideally, above every step, there should be a comment (like `gal_units_extract_decimal').
2. Please review the Gnuastro Coding conventions and look at the sources of some of the libraries. Then implement them in your code. For example note that no line should be longer than 75 characters (you can break the literal strings, see examples in the code). Or that there should a fixed space between the functions, also look at how the functions are described in comments above them.
3. For printing a warning, use the error function, it has some very good advantages compared to an `fprintf' and in Gnuastro it can even be used on systems that don't have the GNU C Library (because Gnuastro uses the GNU Portability Library.
4. This will be a new library in Gnuastro, so we need to add a sub-section under the Gnuastro library section. I can do that.
5. For now, you can simply put this `units.c' (with a corresponding `units.h') under the `lib/' (and `lib/gnuastro/') directories (like the others) and start committing your work. When it's done I can do the necessary steps for Gnuastro to automatically build it.

Until your copyright form is signed, now that you have the core functions, you can look into the Table program and see how you can add an operator in its Column arithmetic feature to do this operation on the columns of an input table.

I have uploaded a new file based on your guidelines.

(file #48616)

One final, and VERY IMPORTANT note: your code didn't have a single comment line! This is simply not acceptable in any large project! You need to describe every step/choice thoroughly. Even describing the peculiar behaviors of `strtod' and `strtok' in your particular usage.

If you are not yet convinced of the importance of comments, you may enjoy this word of wisdom ;-).

Please have a look at this implementation of the parsing function, which I just tried in `units.c'. Please look at the differences one by one and hopefully you'll see what I mean ;-). 

int
gal_units_extract_decimal (char *convert, double *args)
{
  size_t i=0;
  const char delimiter = ':';
  char *token, *copy, *end=NULL;

  token=copy=strdup(convert);
  while(*token!='\0')
    {
      if(i==3)
        {
          fprintf(stderr, "string should only contain 3 numbers\n");
          return 0;
        }

      /* Separate the token and read it as a number. */
      token = strtok(i==0 ? copy : NULL, &delimiter);
      if(token)
        {
          args[i++] = strtod(token, &end);
          if(*end=='\0' || *end==delimiter)
            token=end+1;
          else
            {
              fprintf(stderr, "element %zu of `%s' couldn't be "
                      "parsed as a number\n", i, convert);
              return 0;
            }
        }
    }

  if(i!=3)
    {
      fprintf(stderr, "`%s' must contain three numbers, but has "
              "%zu numbers\n", convert, i);
      return 0;
    }
  return 1;
}

Typos happen a lot while you are running a command and you should account for all scenarios. Also, as I mentioned before, merely checking the length of a string is a very poor quality check! For example `2' is as valid in this scenario as `02'. But in your implementation, the former would crash with an error! You should let the `strtok' and `strtod' decide if what they are given is reasonable or not. Manual checks of the length to be 8 and 2 is not a good solution!

To make this function more useful (in a generic scenario), you can add another argument to it which is the number of expected numbers (which also corresponds to the amount of allocated space in the output array).

Thanks for making the code so easy to test. I was able to extract your desired results with a simple `gcc units.c && ./a.out' command.

The point is that having so many repetitive elements in the code is prone to SO MANY bugs! Generally, it also makes the code VERY HARD to read for the human reader that wants to debug it.

If you add a simple loop to call `strtok', you can add a counter and print the proper error message based on the value of that counter. This will significantly decrease the size of your code and make it easier for others to debug/improve.

On a similar idea: to abstract/remove the repetitions, you can define a lower-level function to just parse the string and extract the three numbers (the part that has to be done for both RA and Dec) and return three floating point numbers (as pointer arguments). Then each of the RA and Dec functions (or any other future function that may have a similar notation) can do what ever they want with the numbers.

Mixing low-level (string parsing/extraction) and high-level (number arithmetic) operations is generally not a good idea ;-).

Thanks Katrik. I see so many tests have been added.

To test it independently, I had to remove the `config.h' part and also add a `main' function:

int
main(void)
{
  char *test="2:30:20";
  printf("%f\n", gal_units_convert_ra_decimal(test));
  printf("%f\n", gal_units_convert_dec_decimal(test));
}

I then gave the first random number that occurred to me: `2:30:20', but I got the `Invalid length' error! I then changed it to `02:30:20' and got that error again.

Generally, this type of error checking on the raw string, and manually checking every number (with the `strtod (copy + 1, &copy)' part) is not good practice: it can hide many bugs.

What I recommend is to use the C library's `strok' function (for GNU C Library, its explained under the Finding tokens in a string section). You can call it in a loop (so all the values are checked in one piece of code). You can specify the delimiter with `strtok' to be `:'.

Thanks a lot Kartik. It look good for the first round, I think it can be merged soon ;-).

To merge it into the master branch, its good to add some sanity checks: typos can happen a lot and the program needs to check all possible scenarios. For example, what if someone only gives, one, two, or 6 numbers (forgets to put a space between RA and Dec).

Other conditions to check are when the ranges are not acceptable.
Its always good to put some checks like this and return absurd values (in this case NaN would be good) while printing a clear warning on `stderr' (including the original string and what the problem is).

Also, before importing it into the Gnuastro library, please look over some of the Gnuastro library source files to get a feeling of our style. Generally, if you haven't done so already, please see the Developing chapter.

For example, let's assume this library will be called with `units.h'. So all the functions that are exposed to the outside world (aren't static) should start with `gal_units_*'. Also note how in the GNU notation, the returned type is on a separate line just before the function name. Please see the Coding conventions. We also only use the old-style comments.

Essentially you don't need anything beyond the standard C library. You just parse the string (with 'strtok'), pull out the tree numbers, convert them to double (with 'strod'), and do the conversion :-)

Great! I'll do that.

The Right ascension (RA) and Declination (Dec) of astronomical objects is sometimes written as `hh:mm:ss' (for RA) and `dd:mm:ss' (for Dec). But in some contexts we need a single standard number (in degrees for example).

We should add a Column Arithmetic operator to facilitate doing this.

But the necessary functions to convert these strings to degrees should be written as a very generic library (taking a string, and returning a double). This will allow us to later use it when reading table columns generally and doing the conversion automatically.