taskGNU Astronomy Utilities - Tasks: task #15109, Arithmetic not reading data when...

 
 

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

task #15109: Arithmetic not reading data when metadata requested

Submitter:  Mohammad Akhlaghi <makhlaghi>
Submitted:  Sat 08 Dec 2018 04:11:07 PM UTC
   
 
Should Start On:  Fri 07 Dec 2018 11:00:00 PM UTC Should be Finished on:  Fri 07 Dec 2018 11:00:00 PM UTC
Category:  Arithmetic Priority:  5 - Normal
Item Group:  Enhancement Status:  Postponed
Privacy:  Public Assigned to:  None
Percent Complete:  0% Open/Closed:  Open
Effort:  0.00

Jump to the original submission

Sun 08 Mar 2020 02:20:55 PM UTC, comment #17: 

Great! your commit is now in the Gnuastro's main `master' branch. Some further corrections were necessary, which I implemented in the next commit. Please go through those points and look at the corrected lines to hopefully prepare you for your next edits in the code ;-).

But as you may have noticed, I just changed the task summary/title, I didn't close it. The reason is that we still need to read-in the full dataset to just read its size. However, as mentioned in the first comment of this task, we don't want this: when only the dataset's metadata is necessary, we shouldn't read the data (and waste time and memory). So to complete this task, we should add some special steps in the arithmetic program so when meta-data are necessary, it doesn't go reading-in the full dataset. For example a special variable/flag called `meta' can be defined in the relevant functions. The  `size' operator (or other later operators) will set it to 1, while the others will set it to zero. When its 1, then the step where we pop the operator (either read it, or just pick it from the stack), we just read/copy the metadata in an empty-of-data dataset.

But this may be a little too low level for you Sachin and isn't a critical issue now. So I recommend moving onto another task/bug. Thanks again for the good work ;-).

Mohammad Akhlaghi <makhlaghi>
Group administrator
Sun 08 Mar 2020 05:04:49 AM UTC, comment #16: 

The final size function is ready along with documentation. I've rebased all previous commits onto a clean one. I've improved it so that no printing of the dimensions is required and a single integer output is returned.
https://github.com/sachinkumarsingh092/gnuastro/tree/task-arithop-size

Sachin Kumar Singh <sks_15>
Sat 07 Mar 2020 03:45:58 PM UTC, comment #15: 

Sorry! In the previous point, I should have written: "once you know the dataset's TYPE, ...". I mistakenly wrote "size".

Mohammad Akhlaghi <makhlaghi>
Group administrator
Sat 07 Mar 2020 03:44:17 PM UTC, comment #14: 

I recommend reading through the data.h documentation, with its subsections. All Gnuastro programs use this structure for reading/writing datasets of any type and size, with some basic metadata.

The `gal_data_alloc' function does the allocation of the necessary space in memory (for the given type and number of elements it has). You just have to write into it.

A simple/readable way to read/write to the datasets' arrays is similar to what we discussed before about the `arg' dataset: once you know the dataset's size, you define a pointer of that type, then set the pointer to point to the dataset's allocated space for data. For example, if you want it to be `size_t' (the type for "size"-related things that can never be negative), you can do this:


size_t *out_arr;
out_arr=out->array;
out_arr[0]=in->dsize[arg_val-1];


But please read the data.h documentation first to avoid future confusions ;-).

Mohammad Akhlaghi <makhlaghi>
Group administrator
Sat 07 Mar 2020 11:27:37 AM UTC, comment #13: 

When using `out->array=(void *)&(in->dsize[arg_val-1])` it gives a corruption error. Also using

out->array = malloc( sizeof unsigned long * );
*(size_t *)t.out->array = in->dsize[arg_val-1];


is showing a corruption too. How to store the dimension in the out->array? Or should I store it down in some other variable and pass it to out somehow>

Sachin Kumar Singh <sks_15>
Sat 07 Mar 2020 03:12:25 AM UTC, comment #12: 

Another small point: in the end (when you rebase the commits into one commit with a good comment), also remove the changes in `tests/during-dev.sh': in the master branch, this file should always be clean.

Mohammad Akhlaghi <makhlaghi>
Group administrator
Sat 07 Mar 2020 02:53:27 AM UTC, comment #11: 

This is good progress indeed. There are just some (mostly minor) points that can be fixed easily but are important:

  • What is the `.vscode/settings.json'? Why are the "associations" only defined for three header files? Is it later necessary to do this for all header files? When you add new files, you need to put a comment describing what it is (inside the file itself), and also include a copyright statement (like any other file in Gnuastro ;-)).
  • As described in Gnuastro's Coding conventions, "One group of contextually similar functions in a source file is separated from another with 20 blank lines." You have put a nice "size" title with two rows of asterisk above the new operator, but it s only a hand-ful of lines distant from the previous function. In order for the code to be coherent and readable, its necessary that all coders follow the same conventions.
  • Similar to the case above, the indentation and positioning of the braces don't follow the GNU standard. Please have a look at some of the other functions to see it in practice.
  • This is even visible in a `git diff' on the commandline: lines shouldn't end with white-space characters. Git highlights them in read by default when you use something like this: `git diff master...task-arithop-size').
  • Maybe you can change the "size" title (in the asterisk rows) to  something like "metadata". For example later, we might add operators to read the type of a dataset.
  • I don't understand why the `out' dataset is allocated with the same size as the input (which can be a large image, thus wasting a lot of memory). The output of this function should just be a single-element dataset (a single number).
  • Finally, don't forget to document your new operator ;-) in the Arithmetic Operators section of the book. The source of the book is in `doc/gnuastro.texi' (written in GNU Texinfo). In essence, its just TeX, but with `@' instead of `\'. You can use the documentation of the other operators as a reference.
Mohammad Akhlaghi <makhlaghi>
Group administrator
Fri 06 Mar 2020 12:02:09 PM UTC, comment #10: 

All the problems regarding the size operator are fixed now. I've pushed the code to my GitHub repo in the `task-arithop-size` branch.

It prints the value of the required dimension along with a fits file containing the metadata. Flags can be used to remove the metadata fits file.
I think it's okay now to push it into the master branch. You can review it and make suggestions and comments.
The repo is here https://github.com/sachinkumarsingh092/gnuastro/tree/task-arithop-size

Sachin Kumar Singh <sks_15>
Fri 06 Mar 2020 01:25:57 AM UTC, comment #9: 

In the previous post I forgot to mention how you can read the contents of `arg' after doing basic sanity checks.

Again, you can find the way by inspecting other functions, but generally, this is what I usually do: 1) define a new `size_t' variable, maybe `arg_val'. 2) Convert `arg' to the `GAL_TYPE_SIZE_T' TYPE (with the `gal_data_copy_to_new_type_free' function). Note that freeing should only be done if the freeing flag is set. 3) Now that you know the type of the `arg->array' pointer, just read/copy it into the variable with something like this: `arg_val=(size_t *)(arg->array)'. Ofcourse you can also directly read `arg->array' as a size_t array, its just more clear for a reader if you use a clearly-named variable.

Mohammad Akhlaghi <makhlaghi>
Group administrator
Fri 06 Mar 2020 01:19:09 AM UTC, comment #8: 

Looks good! You are on the right track :-)!

About reading the contents of the `arg' argument, you can look at a few other functions in that file for examples. For example see `arithmetic_bitwise_not' which does bitwise operations on its input. But its input must be an integer type, so before it starts, it first checks if the `type' element of the `gal_data_t' structure has a floating point type or not. In your case, you also want it to be an integer, so you can add a similar test. But besides it type, you also want it to be a single number. If the user mistakenly gives an array/image as the first argument, your function should crash with a clear error message, like the other functions in `lib/arithmetic.c'.

About the crash with an unrecognized operator, I would recommend to follow the functions from the again. You will find the place that you still haven't added the new operator. I'll let you find it as an exersize ;-). TIP: you can use `grep' with the fixed parts of the error message to find it.

Mohammad Akhlaghi <makhlaghi>
Group administrator
Wed 04 Mar 2020 07:42:15 AM UTC, comment #7: 

For the second argument(the required dimension, let it be called `arg`), I've used `gal_data_t' as well. To use this value, I've used its arg->array, because on inspection it showed a size to be 1, which is desired. But I'm aware it is a void pointer and on dereferencing with a long typecast is showing a large random number. How to fix this? how can I get the value of the second operator?

Also on testing with `during-dev.sh`, it is reported that the `size` is not a valid operator. But the printf debugger works fine and shows all dimensions correctly. I've pushed the repo on GitHub:https://github.com/sachinkumarsingh092/gnuastro/tree/task-arithop-size

Sachin Kumar Singh <sks_15>
Sat 29 Feb 2020 08:08:18 PM UTC, comment #6: 

The functions to deal with FITS images are lib/fits.h, described in the Fits files section of the Gnuastro library section of the manual. You can also access these sections with these commands on the command-line (if you have installed Gnuastro):


info gnuastro "FITS files"
info gnuastro "gnuastro library"


But note that in the Arithmetic program you can't assume the inputs are always FITS files. The users may have done some operations on the file (it is already in memory for example) before asking its size. Because some operators change the dataset's size.

Generally, if you look in the Arithmetic program's source, you'll notice that the Arithmetic program uses the `operand_pop' function (see bin/arithmetic/operands.h to access the operands. Its the job of that function to read the FITS files if the operand is an un-opened FITS file.

In `lib/arithmetic.c', it assumes the dataset is already in memory. So in practice, you just have to pop the top operand in the Arithmetic program, then in the Arithmetic library, you just need to read its size (which is already in the `dsize[]' element of the `gal_data_t').

Mohammad Akhlaghi <makhlaghi>
Group administrator
Sat 29 Feb 2020 01:50:59 PM UTC, comment #5: 

Which function to use to read the fits file or its metadata in the lib/arithmetic.c using a filename? I see the functions in the bin/fits/keywords.c to print all the info but how to use them. Is there any funtion to directly read the hdus or fits?

Sachin Kumar Singh <sks_15>
Wed 26 Feb 2020 02:38:58 PM UTC, comment #4: 

This is an identical problem with bug #57891! Can you follow the discussion there please? You can also try the recommendations there for a start.

But if I am not mistaking, you had successfully built the bootstrapped Gnuastro before, right? (please reply there)

Mohammad Akhlaghi <makhlaghi>
Group administrator
Wed 26 Feb 2020 01:55:54 PM UTC, comment #3: 

I was getting the following error(in test.log) when I made a small change in the size function. I stashed and removed the directory to my last working commit and this still persists. I looked at the `gsl_fft_complex_wavetable_alloc` it seems to be fine. I did a developer build again but this still persists.


(file #48500)

Sachin Kumar Singh <sks_15>
Sun 23 Feb 2020 11:51:43 PM UTC, comment #2: 

I tried the build and it didn't crash, although it did give a "unused variable ‘dsize’" warning, which I assume will be fixed later once you implement the insides of the function.

Let me address these questions in sr #110195: "What are other things I need to change and are the current changes right? What else is to be done? What are the things I'm getting wrong? Any suggestion?"

You have taken the right first steps, that's good! So I recommend looking around a little more in the similar functions of other operators and completing it.

Don't worry about the optimization in the first phase ;-). Just do a basic implementation that does the job (even if you have to load the whole image!). Once it works (and you master the whole process), then start optimizing ;-).

Mohammad Akhlaghi <makhlaghi>
Group administrator
Sun 23 Feb 2020 03:18:19 PM UTC, comment #1: 

I've pushed the changes here https://github.com/sachinkumarsingh092/gnuastro/tree/task-arithop-size .

I've, for now, commented the return types and hoping to create something like this. The error I get is that the `size` operator is not being recognized but I'd also like suggestions about the implementation and possible changes.

Sachin Kumar Singh <sks_15>
Sat 08 Dec 2018 04:11:07 PM UTC, original submission:  

In some processing situations, the size of a dataset along a dimension is necessary. So Arithmetic can use a new operator, maybe called "size" that will do this job. For example the command below will print the size of the dataset along its second dimension.


$ astarithmetic image.fits 2 size


The only thing I am thinking about is that an operator like this (and maybe others like some future ones on printing the type) only need the dataset's meta-data and don't actually need the dataset's contents! So to be most efficient (especially on large datasets), we should implement a mechanism that if reading from a file, we don't need to actually load the contents and if reading from the stack (dataset already in memory, output of previous operator), we don't make a full copy.

Mohammad Akhlaghi <makhlaghi>
Group administrator

 

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

Attached Files
file #48500:  test.log added by sks_15 (3KiB - text/x-log)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by sks_15 (Posted a comment)
  • -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 2 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-03-08 makhlaghi SummarySize of dataset as an Arithmetic operator Arithmetic not reading data when metadata requested
    2020-02-26 sks_15 Attached File- Added test.log, #48500

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code