taskGNU Astronomy Utilities - Tasks: task #15567, Converting hh:mm:ss and dd:mm:ss...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

task #15567: Converting hh:mm:ss and dd:mm:ss notations to degrees and vice vesa

Submitter:  Mohammad Akhlaghi <makhlaghi>
Submitted:  Tue 03 Mar 2020 12:31:31 PM UTC
   
 
Should Start On:  Tue 03 Mar 2020 12:00:00 AM UTC Should be Finished on:  Tue 03 Mar 2020 12:00:00 AM UTC
Category:  Table Priority:  5 - Normal
Item Group:  Enhancement Status:  Done
Privacy:  Public Assigned to:  None
Percent Complete:  100% Open/Closed:  Closed
Effort:  0.00

Jump to the original submission

Thu 09 Apr 2020 02:42:30 AM UTC, comment #57: 

Thanks Kartik. It has been fully merged into the master branch and pushed to the main repository. I went over the changes and made some necessary corrections which can you see in Commit 4154cba9a578.

However, I had to manually add your email to the commit message! This was very annoying, especially since I had already done some of the changes in the next commit when I noticed this! So your original commit has has changed (because the email has changed), but you are ofcourse still the main author of that commit.

Please delete original branch from your remote so I can fetch your remove and remove that reference from my history.

Also, I think it will be very instructive if you look at the changes I made to your original in [the next commit.

Mohammad Akhlaghi <makhlaghi>
Group administrator
Wed 08 Apr 2020 02:56:52 PM UTC, comment #56: 

I have addressed the issues and pushed the fixed commit.

Kartik Ohri <lucifer13>
Tue 07 Apr 2020 10:07:25 PM UTC, comment #55: 

Thanks Kartik. I pulled the changes but still see that the commit message title is two lines! Please correct it to be a single and complete line that is shorter than 75 characters (which can be readable with `git log --oneline').

In the two `gal_units_decimal_to_dec' and `gal_units_decimal_to_ra', I noticed that you have defined the variables in the middle of the functions. This isn't a technical problem in modern C compilers, but it is a bad style ;-)! It makes it so so hard to read the code!

It is much better if all the variables are defined at the top of the function, before any operation is done. When reading a code, someone can simply pull to the top of the function and see all the various types used and variables defined (which is itself valuable information before going into the details).

In the same two functions, the last element (`seconds' and `arc_seconds') is floating point: in many cases we have sub-arcsecond accuracy ;-).

Mohammad Akhlaghi <makhlaghi>
Group administrator
Tue 07 Apr 2020 07:34:45 AM UTC, comment #54: 

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

Kartik Ohri <lucifer13>
Tue 07 Apr 2020 02:24:54 AM UTC, comment #53: 

I pulled the commit, and when looking at it, I couldn't go beyond the commit message!!!

I really want to merge this nice work into the master branch, but it is frustrating that we are stuck behind these small corrections.

Did you read the guidelines?

Did you run `git log' after your commit? Do you see the difference of your commit message with the rest?

Why does the message have two titles? Why is the second title (which should have been the main one), more than one line? Why is there an indentation in the message?

After the message is corrected, please run `git log' and compare the formatting of your commit message with the commits after it. Please only push your changes when it looks similar (following the commit guidelines).

Mohammad Akhlaghi <makhlaghi>
Group administrator
Mon 06 Apr 2020 08:25:23 PM UTC, comment #52: 

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

Kartik Ohri <lucifer13>
Mon 06 Apr 2020 06:13:12 PM UTC, comment #51: 

I fetched your commit, thanks for rebasing it.

I had a fast look at it and see that you haven't implemented the first two points I mentioned before!!! Please only say that you "have made the changes" when you have actually done so ;-). Please inspect the full list one more time and make sure they are implemented.

I think we had discussed the Commit guidelines before, they are in the book. After reading them please do a `git log' on the master branch and see many of the top commit messages as an example ;-).

Mohammad Akhlaghi <makhlaghi>
Group administrator
Mon 06 Apr 2020 05:08:11 PM UTC, comment #50: 

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

Kartik Ohri <lucifer13>
Mon 06 Apr 2020 01:33:06 AM UTC, comment #49: 

Thanks Kartik, it looks good overall, you can now rebase it over the most recent commit, and into a single commit so I do a final test and merge it.

There are just some small corrections

  • Please remove the extra change in the top-level `developer-build' script. If you need to set LDFLAGS on your computer, you can define and export it in your `.bashrc' file so its always available ;-). See the discussion on the Installation directory section of Gnuastro's book.


  • Also, please remove any changes in `tests/during-dev.sh'.


  • In `lib/arithmetic.c', when you import `units.h', please respect the style: as explained in the Coding Conventions: when order doesn't matter, put it in the middle of the other Gnuastro headers such that they are sorted by lenght of line. This is purely stylistic, but helps in visual inspection, compared to a zig-zag style.


  • Again in `lib/arithmetic.c', I noticed that you had moved a few `default' statements to the start of the line, against the standard indentation. Please correct this ;-).


  • In `arithmetic_unary_function', where you check if the new operators with floating point types, please put the `&&'s at the start of each line, just like every where else ;-). This is suggested as part of the GNU Coding Standards, and greatly helps in readability.


  • A few lines under that, please remove the empty line between the comment and the code.
Mohammad Akhlaghi <makhlaghi>
Group administrator
Sun 05 Apr 2020 03:07:03 PM UTC, comment #48: 

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

Kartik Ohri <lucifer13>
Sat 04 Apr 2020 04:38:36 PM UTC, comment #47: 

I have added the facility to convert declination to decimal in the same way as right ascension and pushed to github.

Kartik Ohri <lucifer13>
Mon 30 Mar 2020 10:41:09 AM UTC, comment #46: 

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.

Kartik Ohri <lucifer13>
Sun 29 Mar 2020 12:41:48 AM UTC, comment #45: 

When you want to quote code, please follow the Savannah's Markup system so it is highlighted differently compared to your plain text message on Savannah, look under "Verbatim markup (useful for code bits)" for putting code.

What you quoted from Valgrind is no longer a segmentation fault. Look in Comment #38, it says "Invalid read of size 1" (which can cause a segmentation fault).

The valgrind output you are reporting is a simple memory leakage (which GNU/Linux operating systems automatically free as soon as the program is finished). Ideally we would not want to have any such leakage, so its good if it is removed, but in practice, its not an error. It should go by using `gal_list_data_free' on the table, after it has been saved to the output file (or printed on the command-line). You can fix it in your commit, try it out ;-).

Mohammad Akhlaghi <makhlaghi>
Group administrator
Sat 28 Mar 2020 08:56:32 PM UTC, comment #44: 

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 ?

Kartik Ohri <lucifer13>
Fri 27 Mar 2020 08:13:31 PM UTC, comment #43: 

My copyright forms have gone through. But I wanted to ask do the copyright forms have to be in my real name or can I use an alias ?

Kartik Ohri <lucifer13>
Tue 24 Mar 2020 11:06:38 PM UTC, comment #42: 

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 ;-).

Mohammad Akhlaghi <makhlaghi>
Group administrator
Tue 24 Mar 2020 08:07:54 PM UTC, comment #41: 

Sorry for the typo earlier. I wanted to say that I have fixed the errors from units.c. Please take a look at the update code again. The error I posted was coming from checkset.c. I'll take a look and try to fix this as well.

Meanwhile, I'll begin work on my proposal. I want to write a Julia wrapper for GNUAstro. I wanted to ask if you have any requirements for the wrapper library. I am actually planning to submit a small working demo with the proposal. something like making at least one program work as a demonstration.

Kartik Ohri <lucifer13>
Tue 24 Mar 2020 07:42:32 PM UTC, comment #40: 

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 ;-).

Mohammad Akhlaghi <makhlaghi>
Group administrator
Tue 24 Mar 2020 04:42:44 PM UTC, comment #39: 

I have fixed the issues. Please take a look at the error again. Valgrind still reports two errors but i don't its an issue with units.c .
Here is the log,
==5452== HEAP SUMMARY:
==5452==     in use at exit: 114 bytes in 4 blocks
==5452==   total heap usage: 1,371 allocs, 1,367 frees, 127,389 bytes allocated
==5452==
==5452== 2 bytes in 1 blocks are definitely lost in loss record 1 of 3
==5452==    at 0x483A7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==5452==    by 0x33F24C: gal_checkset_allocate_copy (checkset.c:154)
==5452==    by 0x4C0C47: gal_type_from_string (type.c:492)
==5452==    by 0x360FB7: gal_options_read_check (options.c:1520)
==5452==    by 0x3617D2: options_set_from_name (options.c:1857)
==5452==    by 0x361A44: options_parse_file (options.c:1949)
==5452==    by 0x361C7C: gal_options_parse_config_files (options.c:2034)
==5452==    by 0x3620B2: gal_options_read_config_set (options.c:2150)
==5452==    by 0x11084F: ui_read_check_inputs_setup (ui.c:1104)
==5452==    by 0x10D53D: main (main.c:46)
==5452==
==5452== 112 (32 direct, 80 indirect) bytes in 1 blocks are definitely lost in loss record 3 of 3
==5452==    at 0x483A7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==5452==    by 0x10DC43: ui_outcols_add_new_to_end (ui.c:426)
==5452==    by 0x10DF71: ui_columns_prepare (ui.c:567)
==5452==    by 0x10F21B: ui_preparations (ui.c:973)
==5452==    by 0x11088B: ui_read_check_inputs_setup (ui.c:1125)
==5452==    by 0x10D53D: main (main.c:46)
==5452==
==5452== LEAK SUMMARY:
==5452==    definitely lost: 34 bytes in 2 blocks
==5452==    indirectly lost: 80 bytes in 2 blocks
==5452==      possibly lost: 0 bytes in 0 blocks
==5452==    still reachable: 0 bytes in 0 blocks
==5452==         suppressed: 0 bytes in 0 blocks

Kartik Ohri <lucifer13>
Mon 23 Mar 2020 05:50:40 PM UTC, comment #38: 

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 ;-).

Mohammad Akhlaghi <makhlaghi>
Group administrator
Mon 23 Mar 2020 07:44:48 AM UTC, comment #37: 

Hi! I have fixed this issue. The function was defaulting to GAL_TYPE_FLOAT32 which was causing the issue. I have added an extra check and it works. Instead of that, using float in the units.c might work as well. Please test this again and let me again.

Kartik Ohri <lucifer13>
Sun 22 Mar 2020 07:59:38 PM UTC, comment #36: 

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.

Mohammad Akhlaghi <makhlaghi>
Group administrator
Sun 22 Mar 2020 04:25:21 PM UTC, comment #35: 

Yeah sure, I'll try to debug this myself. As of now from the error log and print statements, I suspect there must be some issue in the applying the arithmetic value on the column. I want to ask if there is a way to give a string input to astarithmetic ?

Kartik Ohri <lucifer13>
Sun 22 Mar 2020 01:32:23 PM UTC, comment #34: 

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.

Mohammad Akhlaghi <makhlaghi>
Group administrator
Sun 22 Mar 2020 01:07:17 PM UTC, comment #33: 

Hi! I tried using valgrind got the attached error log.

(file #48649)

Kartik Ohri <lucifer13>
Sat 21 Mar 2020 07:44:11 PM UTC, comment #32: 

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.

Mohammad Akhlaghi <makhlaghi>
Group administrator
Sat 21 Mar 2020 07:36:30 PM UTC, comment #31: 

I have attached the table.txt file . I ran asttable table.txt --tableformat=txt --minmapsize=10000 --searchin=name -c1,2 -c"arith c2 ra-to-decimal and received the following output.
1      02:30:15 0            
2      05:12:45 3.04346      
3      12:32:11 0            
4      05:12:45 3.30542      
5      23:59:11 5.01922e+33  
free(): invalid next size (fast)
Aborted (core dumped)
So, it is not working. I hope you can guide me with what could potentially be the issue.

(file #48635)

Kartik Ohri <lucifer13>
Sat 21 Mar 2020 05:37:06 PM UTC, comment #30: 

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 ;-)

Mohammad Akhlaghi <makhlaghi>
Group administrator
Sat 21 Mar 2020 09:51:50 AM UTC, comment #29: 

I made the changes you asked. I have added a new macro in arithmetic.c for handling the string arguments in function. I am trying to run make on the project but I get the following error
make  all-recursive
make[1]: Entering directory '/home/amcap1712/gnuastro'
Making all in bootstrapped/lib
make[2]: Entering directory '/home/amcap1712/gnuastro/bootstrapped/lib'
make  all-recursive
make[3]: Entering directory '/home/amcap1712/gnuastro/bootstrapped/lib'
make[4]: Entering directory '/home/amcap1712/gnuastro/bootstrapped/lib'
make[4]: Nothing to be done for 'all-am'.
make[4]: Leaving directory '/home/amcap1712/gnuastro/bootstrapped/lib'
make[3]: Leaving directory '/home/amcap1712/gnuastro/bootstrapped/lib'
make[2]: Leaving directory '/home/amcap1712/gnuastro/bootstrapped/lib'
Making all in lib
make[2]: Entering directory '/home/amcap1712/gnuastro/lib'
make[2]: Nothing to be done for 'all'.
make[2]: Leaving directory '/home/amcap1712/gnuastro/lib'
Making all in bin/arithmetic
make[2]: Entering directory '/home/amcap1712/gnuastro/bin/arithmetic'
/bin/bash ../../libtool  --tag=CC   --mode=link gcc  -Wall -O3  -pthread -L\../../lib  -o astarithmetic main.o ui.o arithmetic.o operands.o ../../bootstrapped/lib/libgnu.la -lgnuastro  -lpthread
libtool: link: gcc -Wall -O3 -pthread -o .libs/astarithmetic main.o ui.o arithmetic.o operands.o  -L../../lib ../../bootstrapped/lib/.libs/libgnu.a /home/amcap1712/gnuastro/lib/.libs/libgnuastro.so -lpthread -pthread
/usr/bin/ld: /home/amcap1712/gnuastro/lib/.libs/libgnuastro.so: undefined reference to `gal_units_ra_to_decimal'
collect2: error: ld returned 1 exit status
make[2]: * [Makefile:2002: astarithmetic] Error 1
make[2]: Leaving directory '/home/amcap1712/gnuastro/bin/arithmetic'
make[1]: * [Makefile:2101: all-recursive] Error 1
make[1]: Leaving directory '/home/amcap1712/gnuastro'
make: * [Makefile:1991: all] Error 2

How to fix the error ? Is it possible to run table program without compiling the whole library ? I have pushed my latest changes so that you can check them out.

Kartik Ohri <lucifer13>
Fri 20 Mar 2020 10:27:39 PM UTC, comment #28: 

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 ;-).

Mohammad Akhlaghi <makhlaghi>
Group administrator
Fri 20 Mar 2020 09:29:59 PM UTC, comment #27: 

I have made the changes to the repo. I have reset my master branch and created a new units branch to make the changes. You can check the repo again at the same link.

Kartik Ohri <lucifer13>
Fri 20 Mar 2020 05:39:19 PM UTC, comment #26: 

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 ;-).

Mohammad Akhlaghi <makhlaghi>
Group administrator
Fri 20 Mar 2020 03:29:00 PM UTC, comment #25: 

Hi, Here is the link https://github.com/amCap1712/gnuastro . Also, is it fine to use an alias instead of my real name?

Kartik Ohri <lucifer13>
Fri 20 Mar 2020 02:47:02 PM UTC, comment #24: 

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.

Mohammad Akhlaghi <makhlaghi>
Group administrator
Fri 20 Mar 2020 02:07:43 PM UTC, comment #23: 

I have made some changes. I have attached a patch so that you can point me in the right direction.

(file #48631)

Kartik Ohri <lucifer13>
Fri 20 Mar 2020 01:44:38 PM UTC, comment #22: 

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.

Kartik Ohri <lucifer13>
Fri 20 Mar 2020 10:58:40 AM UTC, comment #21: 

I have gone through the table arithmetic program. I think what I have do is create a case like a uniary operator in gal_arithmetic function. But since the input column here is a string, whereas the examples I have seen are of numbers. Which file should I edit to add the function ?

Kartik Ohri <lucifer13>
Thu 19 Mar 2020 12:25:41 AM UTC, comment #20: 

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

Mohammad Akhlaghi <makhlaghi>
Group administrator
Wed 18 Mar 2020 07:59:27 PM UTC, comment #19: 

Hi! I have sent an email to -email is unavailable- . Also, I have improved the units.c file as you suggested. I'll take a look into the table program.

(file #48623)

Kartik Ohri <lucifer13>
Wed 18 Mar 2020 03:57:09 PM UTC, comment #18: 

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.

Mohammad Akhlaghi <makhlaghi>
Group administrator
Wed 18 Mar 2020 03:05:17 PM UTC, comment #17: 

I'll look at it shortly, thanks.

But on a side note, have you contacted the GNU Copyright officer? You need to fill some forms (and assign the copyright of your code to GNU) before I can merge it into the master branch. I have described it in the Copyright assignment section of the book.

Please send an email to `assign@gnu.org' and mention that you have written some code that is ready to be merged into Gnuastro. They will guide you on the next steps. Please do this as soon as possible so it can be done faster (it takes some time).

Mohammad Akhlaghi <makhlaghi>
Group administrator
Wed 18 Mar 2020 11:51:41 AM UTC, comment #16: 

I have uploaded a new file based on your guidelines.

(file #48616)

Kartik Ohri <lucifer13>
Tue 17 Mar 2020 09:00:27 PM UTC, comment #15: 

Thank you for the code snippet and detailed advice. It will really help me to write better code in future. I had thought to comment the code once the functions were approved but I will document those side by side from now on. I will send an improved version of the code soon.

Kartik Ohri <lucifer13>
Tue 17 Mar 2020 12:37:32 AM UTC, comment #14: 

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 ;-).

Mohammad Akhlaghi <makhlaghi>
Group administrator
Tue 17 Mar 2020 12:33:29 AM UTC, comment #13: 

Sorry! I forgot to clarify another point: since the parsing function may fail to parse the string, as you have done, it is often necessary to call it with an `if' statement. Its value is also not returned, but written into the argument array.

You can thus use these two points for the benefit of the readability of your code! Notice how in my implementation it just returns an integer. The returned value is 1 if it succeeds and 0 if it fails. So you can call it simply like this:


if(gal_units_extract_decimal(convert, val))
 ....
else
 ....


The statement you had used (below) is very confusing: its returning the opposite of true (0) to be true!


if (gal_units_extract_decimal (convert, val) == 0)


I hope this helps in writing a good code ;-).

Mohammad Akhlaghi <makhlaghi>
Group administrator
Tue 17 Mar 2020 12:27:21 AM UTC, comment #12: 

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).

Mohammad Akhlaghi <makhlaghi>
Group administrator
Mon 16 Mar 2020 06:26:02 AM UTC, comment #11: 

I have separated the low level and high level functions and tried to make improvements in readability.

(file #48604)

Kartik Ohri <lucifer13>
Sun 15 Mar 2020 11:35:21 PM UTC, comment #10: 

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 ;-).

Mohammad Akhlaghi <makhlaghi>
Group administrator
Sun 15 Mar 2020 05:20:50 AM UTC, comment #9: 

I have made the changes. I am using strtok as you suggested but without a loop so that the error messages can be more informative. I have added a main function and removed config.h so that you can test it easily.

(file #48598)

Kartik Ohri <lucifer13>
Sat 14 Mar 2020 09:41:36 PM UTC, comment #8: 

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 `:'.

Mohammad Akhlaghi <makhlaghi>
Group administrator
Sat 14 Mar 2020 09:16:44 PM UTC, comment #7: 

I have improved the code. I have added sanity checks and made the code conform with GNU Coding conventions. Please take a look at the attached file and let me know if any other changes are necessary.

(file #48597)

Kartik Ohri <lucifer13>
Fri 13 Mar 2020 12:39:34 AM UTC, comment #6: 

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.

Mohammad Akhlaghi <makhlaghi>
Group administrator
Tue 10 Mar 2020 06:43:42 PM UTC, comment #5: 

Sorry for the delay. I was caught up a bit in academics. I have uploaded the main.c with the code. Can you please review and provide further instructions ?

(file #48574)

Kartik Ohri <lucifer13>
Tue 03 Mar 2020 06:32:13 PM UTC, comment #4: 

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 :-)

Mohammad Akhlaghi <makhlaghi>
Group administrator
Tue 03 Mar 2020 05:46:42 PM UTC, comment #3: 

I have a few questions. Can GNULib or C standard library functions be used ? Is there anything in particular that I cannot use or I should use ?

Kartik Ohri <lucifer13>
Tue 03 Mar 2020 05:42:23 PM UTC, comment #2: 

Great! I'll do that.

Kartik Ohri <lucifer13>
Tue 03 Mar 2020 05:37:52 PM UTC, comment #1: 

To start with, it is possible to write this function completely independent of Gnuastro: as an independent function that takes a string and returns a double. In a small program as a sibgle file and easily run/test it.

Once it works, we can bring it into Gnuastro.

Mohammad Akhlaghi <makhlaghi>
Group administrator
Tue 03 Mar 2020 12:31:31 PM UTC, original submission:  

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.

Mohammad Akhlaghi <makhlaghi>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #48649:  error.txt added by lucifer13 (5KiB - text/plain)
file #48635:  table.txt added by lucifer13 (119B - text/plain)
file #48623:  units.c added by lucifer13 (5KiB - text/x-csrc)
file #48616:  units.c added by lucifer13 (4KiB - text/x-csrc)
file #48604:  units.c added by lucifer13 (3KiB - text/x-csrc)
file #48598:  units.c added by lucifer13 (4KiB - text/x-csrc)
file #48597:  units.c added by lucifer13 (3KiB - text/x-csrc)
file #48574:  main.c added by lucifer13 (732B - text/x-csrc)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by makhlaghi (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-04-09 makhlaghi StatusNone Done
        Percent Complete0% 100%
        Open/ClosedOpen Closed
    2020-03-22 lucifer13 Attached File- Added error.txt, #48649
    2020-03-21 lucifer13 Attached File- Added table.txt, #48635
    2020-03-20 lucifer13 Attached File- Added 0001-Add-units-library.patch, #48631
    2020-03-18 lucifer13 Attached File- Added units.c, #48623
    2020-03-18 lucifer13 Attached File- Added units.c, #48616
    2020-03-16 lucifer13 Attached File- Added units.c, #48604
    2020-03-15 lucifer13 Attached File- Added units.c, #48598
    2020-03-14 lucifer13 Attached File- Added units.c, #48597
    2020-03-10 lucifer13 Attached File- Added main.c, #48574

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code