taskGNU Astronomy Utilities - Tasks: task #16073, --copykeys of Fits program takes...

 
 

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

task #16073: --copykeys of Fits program takes keyword names also

Submitter:  Mohammad Akhlaghi <makhlaghi>
Submitted:  Thu 11 Nov 2021 11:31:09 PM UTC
   
 
Should Start On:  Thu 11 Nov 2021 12:00:00 AM UTC Should be Finished on:  Thu 11 Nov 2021 12:00:00 AM UTC
Category:  Fits Priority:  5 - Normal
Item Group:  Enhancement Status:  Postponed
Privacy:  Public Assigned to:  None
Percent Complete:  100% Open/Closed:  Closed
Effort:  0.00

Jump to the original submission

Tue 19 Apr 2022 07:07:15 PM UTC, comment #11: 

Congratulations on completing the copyright paperwork Jash!

Your commit has now been merged into the 'master' branch as Commit efdbb7586a, so I am closing the issue!

Your name is now on the second page of the PDF manual (currently the bleeding edge version): http://akhlaghi.org/gnuastro.pdf and you can get the most recent tarball (which now has your newly added feature) from here: http://akhlaghi.org/gnuastro-latest.tar.lz

Thanks also Juan for suggesting this very useful feature :-).

Mohammad Akhlaghi <makhlaghi>
Group administrator
Mon 18 Apr 2022 09:56:42 AM UTC, comment #10: 

Thanks for the comprehensive review! I have gone through all the points you have made and the changes in the code in detail, to understand what could've been improved in my approach.

I'll try to address them as much as I can here:

Regarding the re-writing of the existing implementation:
I'll keep in mind going forward to let improvements and feature addition be just additions, that don't make(or make as minimum as possible) changes to the existing code.

Regarding the issue that the datasets keywords were being re-written into memory, thus creating issues for when users might manipulate the input dataset with other options:
This is definitely a use case that I simply missed. Moving forward, I'll be better about taking such cases into consideration(such edge cases will inevitably become recognizable the more familiar I become with the library).

Regarding the naming convention of keystocopy:
One of the major aspects which first attracted me to Gnuastro was its code and user guide readability. I'll make it a point with future contributions to maintain those same standards.

I've also noted the change in the documentation, comments and code style that you have made.



I will start testing this branch, and notify here if I find any bugs.

I've printed, filled, and submitted the copyright assignment form, they should get back to me soon.

Jash Shah <jash_shah>
Mon 18 Apr 2022 12:44:11 AM UTC, comment #9: 

Thanks a lot Jash, the code has become much better, allowing me to dig into the details. You can see my commit, with a full description of the changes I made in the commit message: https://codeberg.org/gnuastro/gnuastro/commit/e33704e43f0

Please read the commit message carefully and compare all the points mentioned there with the changes in the code. The first and second ones in particular are very important points (in software engineering in general, not just in Gnuastro), so it helps a lot to study them carefully to avoid these pitfalls in the future and be a more powerful developer ;-).

Please build this version of Gnuastro (after pulling that development branch) and try out the '--copykeys' option (both old/new features in complex situations) to see if I have introduced any bugs or not. Also, let me know when the copyright paperwork is complete so I merge it into the 'master' branch.

Otherwise this task is complete (I am just waiting for the copyright paperwork), so I have marked it as 100% complete, but not yet closed.

Mohammad Akhlaghi <makhlaghi>
Group administrator
Wed 06 Apr 2022 08:47:46 PM UTC, comment #8: 

Hey, here's all the updates I made:

  • I've changed my Git commit name.
  • I've updated the code to meet the Coding Conventions now.
  • I've re-designed the code now such that:
    • The main "keywords" function is untouched.
    • I've split the two copying functionalities into two separate functions - "keywords_copykeys_range()" & "keywords_copykeys_str()". So "keywords_copykeys()" only does a call to one of these depending on what input was given by user. This choosing is done by checking whether the string list "keystocopy" is filled or not.
    • While there are some similarities in these two functions, it would become a bit convoluted to fit both these functionalities into one function. Moreover, trying to follow the point in Coding Conventions regarding breaking a function to improve readability, I decided to separate them.
  • I've also updated the gnuastro.texi doc to describe the new functionality added to --copykeys.


I hope this addresses all changes you mentioned. However, please let me know if there is anything more to improve. Even doing this much has helped me understand the philosophy behind GNUAstro a lot :)

Jash Shah <jash_shah>
Tue 05 Apr 2022 07:12:25 AM UTC, comment #7: 

Sorry for all the mistakes, I should've followed the developing chapter more closely (^_^)/.
I will make the necessary changes and inform you once I re-commit!

Jash Shah <jash_shah>
Sat 02 Apr 2022 11:30:33 PM UTC, comment #6: 

Thanks a lot Jash. Here are some comments:

In particular, you will see some of them are stylistic that don't affect the program's running. Nevertheless, when contributing to a new code-base, it is very important to use the existing conventions, otherwise the code will become unreadable as more people contribute ;-).

  • I noticed that your Git commit name is 'Jash-Shah'. Having a SPACE between your first and family names is fine. If "Jash" is your first name and "Shah" your family name, it helps readability (and future inclusion in the second page of Gnuastro's PDF manual) if you use a simple SPACE instead of a dash like the template below (note the SPACE between "your" and "Name"):



git config --global user.name "Your Name"
git config --global user.email "youremail@yourdomain.com"


  • Before pushing your commit, run a 'git log -p -1' and see if Git has marked any of the regions in red. These are common bad programming styles. For example, when a line ends in several white spaces. I saw one such case in your changes in 'ui_check_copykeys'.


  • According to Gnuastro's coding conventions, all lines within the source file should be at most 75 characters. But I saw some very long comment lines in 'ui_check_copykeys'. If you use GNU Emacs as your text editor, you can simply press M-q (or ALT+q on most keyboards) to do this for you.


  • Staying on comments, I noticed that in some places (like above 'keywords_copykeys_str', you had put an empty line before and after the comment description. As you see in the rest of the Gnuastro's code, this is not the convention used in Gnuastro. So please follow a similar convention in all new comments.


  • When defining blocks, you had indented the curly-braces at the same level as the 'if', like the bad code below. But according to the GNU Coding Standards, you should use the good indentation style below. As you read the existing Gnuastro code, you should have noticed how different your code-blocks look ;-). If you use Emacs, simply pressing TAB on each line will fix the indentation to the GNU style.



/* Bad */
if (isalpha(*pt))
{
....
}

/* Good */
if (isalpha(*pt))
  {
    ....
  }


  • In terms of design, I noticed that in the high-level 'keywords' function (of 'bin/fits/keywords.c'), you had implemented a completely independent process for the name-based copying. This is not good practice ;-). We already have all the necessary low-level steps to do the writing in a later part of that function. Duplicating the effort is not good in programming. Also, mixing low-level and high-level steps is not good. You should use the concept of modularity effectively. In other words, each one of the operations in the 'keywords' function is very different from each other, and each have their own internal complexities at a low-level. But when you are looking in the high-level 'keywords', you shouldn't see those complexities! So hide all the complexities of integer-based or name-based keyword copying in the 'keywords_copykeys' function and keep the 'keywords' function untouched ;-).


After you implement these, I'll take a closer look at the lower-level details (this has already become relatively long) ;-).

Mohammad Akhlaghi <makhlaghi>
Group administrator
Sat 26 Mar 2022 04:17:15 PM UTC, comment #5: 

Hey Mohammad,
I've pushed my commit to the branch here.
GitHub Repo Link.

Also,

  • I've changed the condition for identifying a character to use isalpha().
  • I've also tried to make the whole checking more generic, to be able to detect a list of keywords even if the keyword name starts with a string of digits.
    • for this I changed the switch..case to an if..else ladder. So, now it will start parsing through the input string like it used to previously for an INT:INT input, but if it encounters any alphabet then it will instead run the script for STR,STR,.. input.
Jash Shah <jash_shah>
Sat 26 Mar 2022 10:39:54 AM UTC, comment #4: 

Very good progress Jash and congratulations on your first free software contribution :-).

The code generally looks good, but to help me inspect it easily in the middle of the rest of the source, I recommend to build a Git branch and "push" your changes there. A Forking tutorial has been written to help new contributors in doing this.

Now that you have bootstrapped, built, and tested your new function, implementing the forking tutorial will be very easy :-).

Here are just some small tips that I noticed after a fast look in the code in Savannah. I'll have more comments after inspecting your Git branch, since it will be on my own GNU Emacs advanced text editor, which is much more easier to read than a webpage :-):

  • To identify if a character is a number or an alphabetical, instead of directly looking at the ASCII value of a character ('*pt >=65 && *pt<= 90'), its more robust/clean/generic to use the C library's 'isalpha' or 'isdigit' functions (that are described in the GNU C Library manual).
  • Now that I think about it more generally, it may happen that a FITS keyword also starts with a digit. So the most generic test would be to parse through the whole keyword and stop as soon as you find a single non-digit character.


Mohammad Akhlaghi <makhlaghi>
Group administrator
Fri 25 Mar 2022 01:40:58 PM UTC, comment #3: 

Hey @makhlaghi, I've come with a crude solution to this for now, wanted to get your thoughts on it.


I've tried to follow a similar flow as --copykeys=INT:INT does currently.
=== Changes ===

  1. Add a gal_list_str_t *keystocopy to the fitsparams struct.
  2. Update the ui_check_copykeys() fuction in /bin/fits/ui.c to check whether the first character of the entered value of --copykeys is a capital letter(maybe we can instead check if it's a number or not as suggested in your original comment.)

==== ui_check_copykeys() ====

static void
ui_check_copykeys(struct fitsparams *p)
{
  ....

  /* Initialize the values. */
  p->copykeysrange[0]=p->copykeysrange[1]=GAL_BLANK_LONG;

  /* Check if first char of copykeys is a capital letter or number */
  if(*pt >=65 && *pt<= 90){
    /* Parse the copykeys string and store name of each keyword in keystocopy list*/
    char *first = strtok(p->copykeys, ",");

    while(first != NULL){
      gal_list_str_add(&p->keystocopy, first, 0);
      first = strtok(NULL,",");
    }
  }
  else{
    ...Perform steps as normal for INT:INT input...
}


  1. So here, the copykeysrange array elements stay as GAL_BLANK_LONG
  2. In /bin/fits/keywords.c I've added this to the part in main function where, char *inkeys was being set.

==== keywords() ====

/*
   If a list of keys are to be copied then call keywords_copykeys_str
   If a range of keywords must be copied, get all the keywords as a
     single string. */
  if(p->copykeys)
    {
      if (p->keystocopy){
        keywords_copykeys_str(p);
      }
      else{
      keywords_open(p, &fptr, READONLY);
      if( fits_convert_hdr2str(fptr, 0, NULL, 0, &inkeys, &numinkeys,
                               &status) )
        gal_fits_io_error(status, NULL);
      status=0;
      }
    }


00 Also, I've updated the condition for when keywords_copykeys() will be called to avoid calling it if keystocopy exists.

  /* Write desired keywords into output when they are given in range. */
  if(p->copykeys && !p->keystocopy)
    {
      keywords_copykeys(p, inkeys, numinkeys);
      free(inkeys);
    }


  1.  This is the main keywords_copykeys_str() function used for copying data from input file to output.

==== keywords_copykeys_str() ====

/*
To copy keys to output file when --copykeys are given in STR,STR,STR format
*/
static void
keywords_copykeys_str(struct fitsparams *p)
{
  fitsfile *fptr=NULL; /* File pointer used for accessing input and output files*/
  gal_list_str_t *input, *tmp;
  size_t i, nkeys;
  gal_data_t *keysll=NULL;
  int status=0, updatechecksum=0, checksumexists=0;


  printf("Keys to Copy :\n");
  gal_list_str_print(p->keystocopy);
  nkeys=gal_list_str_number(p->keystocopy);

/* Parse  input file, read the keywords and put them in the output (keysll)
  list. */
  input=p->input;

  /* Open the input FITS file. */
  fptr=gal_fits_hdu_open(input->v, p->cp.hdu, READONLY);

  /* Allocate the array to keep the keys. */
  i=0;
  keysll=gal_data_array_calloc(nkeys);
  for(tmp=p->keystocopy; tmp!=NULL; tmp=tmp->next)
    {
      if(tmp->next) keysll[i].next=&keysll[i+1];
      keysll[i].name=tmp->v;
      ++i;
    }

  /* Read the keys.*/
  gal_fits_key_read_from_ptr(fptr, keysll,1,1);

  /* Close the input FITS file. */
  status=0;
  if(fits_close_file(fptr, &status))
    gal_fits_io_error(status, NULL);

    /* Open the output HDU. */
  fptr=gal_fits_hdu_open(p->cp.output, p->outhdu, READWRITE);

  /* See if a 'CHECKSUM' key exists in the HDU or not (to update in case we
     wrote anything). */
  checksumexists=gal_fits_key_exists_fptr(fptr, "CHECKSUM");

  /* Copy the requested headers into the output files header. */
  for(int i=0;i<nkeys;++i)
    {
      char inkey[80];
      char *keyname = keysll[i].name;

      if(fits_read_card(fptr,keyname, inkey, &status))
      gal_fits_io_error(status,NULL);

      if( fits_write_record(fptr, inkey, &status ) )
        gal_fits_io_error(status, NULL);
      else updatechecksum=1;

    }

  /* If a checksum existed, and we made changes in the file, we should
     update the checksum. */
  if(checksumexists && updatechecksum)
    if( fits_write_chksum(fptr, &status) )
      gal_fits_io_error(status, NULL);

  /* Close the output FITS file. */
  status=0;
  if(fits_close_file(fptr, &status))
    gal_fits_io_error(status, NULL);

  /* Clean Up */
  // gal_data_free(keysll[i].next);
}


== Build ==

  1. I've built it on my local bootstraped repo as mentioned here.
  2. also, I've tested it out on the examples given here.


PS :- Sorry for such a long message. This is my first proper opensource project. So I wanted to be as comprehensive as possible (^_^)/

Jash Shah <jash_shah>
Tue 22 Mar 2022 03:16:28 PM UTC, comment #2: 

Great! This will be a very convenient feature to have :-).

It is also fairly simple and a good start into Gnuastro's code-base, for the main Google Summer of Code project.

So this task is now assigned to you, let us know how you progress ;-).

Mohammad Akhlaghi <makhlaghi>
Group administrator
Tue 22 Mar 2022 03:02:16 PM UTC, comment #1: 

Hi, I think I can try adding this feature. It would be great if you could assign it to me.

Thanks :)

Jash Shah <jash_shah>
Thu 11 Nov 2021 11:31:09 PM UTC, original submission:  

Currently the --copykeys option of Gnuastro's Fits program ('astfits') only takes a range of keyword positions and copies the whole patch of keywords to the output (see the description of this option in the book).

However, in some scenarios, the position of the keywords may change in the input, or the required keywords may not be contiguous in the header. So it would be good for this option to accept keyword names also (as a comma-separated list, or possibly called multiple times). For example '--copykeys=ABC,DEF'.

To distinguish between the two scenarios, we can simply see if the first value can be parsed as a number or not. If so, the user intended a range, otherwise, certain keyword names.

This task was suggested by Juan Antonio Fernández Ontiveros.

Mohammad Akhlaghi <makhlaghi>
Group administrator

 

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

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    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 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-04-19 makhlaghi Open/ClosedOpen Closed
    2022-04-18 makhlaghi Percent Complete60% 100%
    2022-03-26 makhlaghi Percent Complete0% 60%
    2021-11-11 makhlaghi Carbon-Copy- Added -email is unavailable-

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code