Reading FITS table columns embedding variable-length arrays #5099

cosimoNigro · 2020-03-07T11:47:14Z

Dear ROOT developers,

this Pull Request tries to address this issue I have opened in the ROOT forum.

A limitation of the TFITSHDU class was its impossibility to read within columns of a FITS table containing a variable-length array. My patch adds such a possibility.

I have modified the Column and Cell structs adding objects necessary to read a variable-length arrays embedded in a cell. TheLoadHDU function (fundamentally the initialiser of the TFITSHDU class) was modified accordingly such that this objects could be assigned.

I have modified the functions dealing with printing and reading values from a table such that the occurrence of a column with variable-length arrays embedded can be properly signalled.

In order not to interfere with the previous development of data handling I created a function to read within a cell for this specific case: it is called GetTabVarLengthVectorCell. I believe it does not make sense to read the entire column in this case (returning an array of variable-length arrays). The user is allowed to access the single cell, when attempting to use any other function (e.g. GetTabRealVectorColumn) to read the entire column, a warning is issued pointing him to this specific function.

Last I added a tutorial in tutorials/fitsio/FITS_tutorial8.C with a test file illustrating the application of this case.

I would kindly ask for a review,

Best

Cosimo

… columns

phsft-bot · 2020-03-07T11:47:16Z

Can one of the admins verify this patch?

cosimoNigro · 2020-03-07T11:51:41Z

graf2d/fitsio/inc/TFITS.h

+      kRealNumber,     // the column contains a scalar
+      kRealArray,      // the column contains a fixed-length array
+      kRealVector      // the column contains a variable-length array
   };


Here I try to use a nomenclature similar to C++ basic objects: a kRealArray is an array, i.e. an object of fixed length, a kRealVector is a vector, i.e. an object whose length is variable. Hope you approve it, the following checks are based on it.

Axel-Naumann · 2020-03-09T13:37:42Z

@phsft-bot build!

phsft-bot · 2020-03-09T13:38:16Z

Starting build on ROOT-performance-centos7-multicore/default, ROOT-fedora27/noimt, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu18.04-i386/cxx14, mac1014/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2020-03-09T13:59:21Z

Build failed on ROOT-fedora30/cxx14.
See console output.

Warnings:

/build/workspace/root-pullrequests-build/root/graf2d/fitsio/src/TFITS.cxx:541:21: warning: declaration of ‘repeat’ shadows a previous local [-Wshadow]

phsft-bot · 2020-03-09T14:01:49Z

Build failed on ROOT-fedora27/noimt.
See console output.

Warnings:

/mnt/build/workspace/root-pullrequests-build/root/graf2d/fitsio/src/TFITS.cxx:541:21: warning: declaration of ‘repeat’ shadows a previous local [-Wshadow]

phsft-bot · 2020-03-09T14:03:22Z

Build failed on ROOT-fedora29/python3.
See console output.

Warnings:

/mnt/build/workspace/root-pullrequests-build/root/graf2d/fitsio/src/TFITS.cxx:541:21: warning: declaration of ‘repeat’ shadows a previous local [-Wshadow]

phsft-bot · 2020-03-09T14:03:46Z

Build failed on ROOT-ubuntu16/nortcxxmod.
See console output.

Warnings:

/mnt/build/workspace/root-pullrequests-build/root/graf2d/fitsio/src/TFITS.cxx:541:21: warning: declaration of ‘repeat’ shadows a previous local [-Wshadow]

phsft-bot · 2020-03-09T14:04:43Z

Build failed on ROOT-performance-centos7-multicore/default.
See console output.

Warnings:

/data/sftnight/workspace/root-pullrequests-build/root/graf2d/fitsio/src/TFITS.cxx:541:21: warning: declaration of ‘repeat’ shadows a previous local [-Wshadow]
/data/sftnight/workspace/root-pullrequests-build/root/graf2d/fitsio/src/TFITS.cxx:385:12: warning: shadowed declaration is here [-Wshadow]

Failing tests:

projectroot.runtutorials.tutorial_fitsio_FITS_tutorial8

pcanal · 2020-03-09T15:25:01Z

tutorials/fitsio/FITS_tutorial8.C

+   TArrayD *arr3 = hdu->GetTabVarLengthVectorCell(rownum, colname3);
+   printf("(%f, %f) \n", arr3->At(0), arr3->At(1)); 
+
+}


Please, add a new line at the end of the file :)

phsft-bot · 2020-03-09T17:40:59Z

Build failed on windows10/cxx14.
See console output.

phsft-bot · 2020-03-09T17:51:56Z

Build failed on ROOT-ubuntu18.04-i386/cxx14.
See console output.

Warnings:

/home/sftnight/build/workspace/root-pullrequests-build/root/graf2d/fitsio/src/TFITS.cxx:541:21: warning: declaration of ‘repeat’ shadows a previous local [-Wshadow]

pcanal · 2020-03-09T19:10:03Z

graf2d/fitsio/src/TFITS.cxx

+
+            for (long row = 0; row < table_rows; row++) {
+               long offset = 0;
+               long repeat = 0;


This variable name (see CI warnings) is already used in prior code. Either reduce the 'lifetime/scope' of the previous variable or rename this one to avoid (future developer reader the code) confusion. thanks.

pcanal · 2020-03-09T19:20:07Z

To 'fix' the Windows CI build, please fork the roottest repository (https://github.com/root-project/roottest)

…in TFITS.cxx

cosimoNigro · 2020-03-10T13:00:53Z

Hi @pcanal,

I have implemented the changes you requested.
I did not understand though your instruction on "fix"ing the windows CI.
I downloaded and run the test suit, shall I share the output somehow?
The result is

74% tests passed, 240 tests failed out of 934

I don't know though if the tests that failed did it for the modification introduced by my patch.

How am I supposed to interpret and share the test output?

Thank you

pcanal · 2020-03-10T14:39:00Z

The Windows CI part requires that your github account has a fork of both the root repository (you already do) but also the roottest repository (you seem to now also do).

pcanal · 2020-03-10T14:39:37Z

@phsft-bot build with flags -DCTEST_TEST_EXCLUDE_NONE=On

phsft-bot · 2020-03-10T14:39:48Z

Starting build on ROOT-performance-centos7-multicore/default, ROOT-fedora27/noimt, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu18.04-i386/cxx14, mac1014/cxx17, windows10/cxx14 with flags -DCTEST_TEST_EXCLUDE_NONE=On
How to customize builds

phsft-bot · 2020-03-10T15:15:48Z

Build failed on mac1014/cxx17.
See console output.

Errors:

1002/2028 Test Correctly set the VFS in the clang FileManager. #910: tutorial-v7-draw_legend.cxx .......................................................................***Failed Error regular expression found in output. Regex=[: error:] 0.69 sec

Failing tests:

pcanal · 2020-03-10T15:27:20Z

The fit tutorials errors is

Processing /build/jenkins/workspace/root-pullrequests-build/root/tutorials/fitsio/FITS_tutorial8.C...
Warning in <TFITSHDU::LoadHDU>: error opening FITS file. Details: could not open the named file
libc++abi.dylib: terminating with uncaught exception of type int
CMake Error at /build/jenkins/workspace/root-pullrequests-build/build/RootTestDriver.cmake:238 (message):
  error code: Child aborted

cosimoNigro · 2020-03-10T15:42:43Z

I am sorry, I forgot to specify the tutorial dir, as at the beginning of the other tutorial scripts

   TString dir = gROOT->GetTutorialDir();
   TFITSHDU* hdu = new TFITSHDU(dir + "/fitsio/rmf_obs5029747.fits", 1);

It could execute only from tutorials/fitsio/.
Apologies

pcanal · 2020-03-10T15:46:43Z

@phsft-bot build with flags -DCTEST_TEST_EXCLUDE_NONE=On

phsft-bot · 2020-03-10T15:46:55Z

Starting build on ROOT-performance-centos7-multicore/default, ROOT-fedora27/noimt, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu18.04-i386/cxx14, mac1014/cxx17, windows10/cxx14 with flags -DCTEST_TEST_EXCLUDE_NONE=On
How to customize builds

phsft-bot · 2020-03-10T18:02:57Z

Build failed on mac1014/cxx17.
See console output.

Errors:

1004/2028 Test Correctly set the VFS in the clang FileManager. #910: tutorial-v7-draw_legend.cxx .......................................................................***Failed Error regular expression found in output. Regex=[: error:] 1.49 sec

Failing tests:

projectroot.runtutorials.tutorial_v7_draw_legend.cxx

phsft-bot · 2020-03-10T18:16:40Z

Build failed on ROOT-fedora30/cxx14.
See console output.

Errors:

1006/2027 Test [cxxmodules] Added missing PushTransactionRAII to rootcling. #912: tutorial-v7-draw_legend.cxx .......................................................................***Failed Error regular expression found in output. Regex=[: error:] 1.22 sec

Failing tests:

projectroot.runtutorials.tutorial_v7_draw_legend.cxx

phsft-bot · 2020-03-10T18:33:28Z

Build failed on ROOT-ubuntu16/nortcxxmod.
See console output.

Failing tests:

projectroot.runtutorials.tutorial_tmva_TMVACrossValidationApplication

pcanal · 2020-03-10T19:09:52Z

The errors are unrelated.

phsft-bot · 2020-03-10T19:47:58Z

Build failed on windows10/cxx14.
See console output.

Errors:

C:\build\workspace\root-pullrequests-build\root\graf2d\fitsio\src\TFITS.cxx(568,30): error C2131: expression did not evaluate to a constant [C:\build\workspace\root-pullrequests-build\build\graf2d\fitsio\FITSIO.vcxproj]
C:\build\workspace\root-pullrequests-build\root\graf2d\fitsio\src\TFITS.cxx(573,28): error C2131: expression did not evaluate to a constant [C:\build\workspace\root-pullrequests-build\build\graf2d\fitsio\FITSIO.vcxproj]
C:\build\workspace\root-pullrequests-build\root\graf2d\fitsio\src\TFITS.cxx(578,30): error C2131: expression did not evaluate to a constant [C:\build\workspace\root-pullrequests-build\build\graf2d\fitsio\FITSIO.vcxproj]
C:\build\workspace\root-pullrequests-build\root\graf2d\fitsio\src\TFITS.cxx(583,31): error C2131: expression did not evaluate to a constant [C:\build\workspace\root-pullrequests-build\build\graf2d\fitsio\FITSIO.vcxproj]

pcanal · 2020-03-10T19:58:30Z

graf2d/fitsio/src/TFITS.cxx

+               // TODO: add all type cases, for now only short, long, float and double are considered
+               //
+               if (abstype == 21) {
+                  short data[size];


The version or setup of Microsoft's compiler on Windows, does apparently not support this construct (array which a 'const int' size). You could try std::vector<short> data; data.resize(sizse);

Also done,
I replaced the data array with a vector, as per your suggestion, and then passed the vector.data() array to the fits_read_col function requiring it.

phsft-bot · 2020-03-11T00:53:39Z

Build failed on ROOT-ubuntu18.04-i386/cxx14.
See console output.

Errors:

1087/1878 Test Refactored dict name checking in TCling [NFC] #820: tutorial-v7-draw_legend.cxx .......................................................................***Failed Error regular expression found in output. Regex=[: error:] 2.13 sec

Failing tests:

cosimoNigro · 2020-03-11T13:08:32Z

Hi @pcanal, I changed the arrays with vectors, could you retry now?

phsft-bot · 2020-03-11T13:08:42Z

Starting build on ROOT-performance-centos7-multicore/default, ROOT-fedora27/noimt, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu18.04-i386/cxx14, mac1014/cxx17, windows10/cxx14
How to customize builds

pcanal · 2020-03-11T16:29:14Z

@phsft-bot build with flags -DCTEST_TEST_EXCLUDE_NONE=On

phsft-bot · 2020-03-11T16:29:22Z

Starting build on ROOT-performance-centos7-multicore/default, ROOT-fedora27/noimt, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu18.04-i386/cxx14, mac1014/cxx17, windows10/cxx14 with flags -DCTEST_TEST_EXCLUDE_NONE=On
How to customize builds

phsft-bot · 2020-03-11T23:34:30Z

Build failed on ROOT-ubuntu18.04-i386/cxx14.
See console output.

Failing tests:

projectroot.runtutorials.tutorial_tmva_TMVACrossValidationApplication

cosimoNigro · 2020-03-12T06:35:11Z

Hi @pcanal, could you wait before merging? I am doing some more tests with a file with larger arrays in columns.
Thank you

cosimoNigro · 2020-03-12T06:59:45Z

Hello,
sorry for this last modification.
The .FITS file I initially decided to include was produced by an experiment specifically for an open-source project and I was not sure it could be made available from another repository (see discussion here, if you are interested).

Therefore I included one of the test files that are shipped with the sherpa X-ray analysis tools.
All NASA mission data are released without license or restriction, so it should be fine to include it in ROOT.

I changed the input file and the tutorial, sorry if you have to run the tests again.
It was an important issue to address.

pcanal · 2020-03-12T18:40:25Z

@phsft-bot build with flags -DCTEST_TEST_EXCLUDE_NONE=On

phsft-bot · 2020-03-12T18:40:34Z

Starting build on ROOT-performance-centos7-multicore/default, ROOT-fedora27/noimt, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu18.04-i386/cxx14, mac1014/cxx17, windows10/cxx14 with flags -DCTEST_TEST_EXCLUDE_NONE=On
How to customize builds

phsft-bot · 2020-03-12T19:11:12Z

Build failed on ROOT-fedora30/cxx14.
See console output.

Failing tests:

projectroot.runtutorials.tutorial_tmva_TMVACrossValidationApplication

phsft-bot · 2020-03-12T19:13:36Z

Build failed on ROOT-ubuntu16/nortcxxmod.
See console output.

Failing tests:

projectroot.runtutorials.tutorial_tmva_TMVACrossValidationApplication

cosimoNigro · 2020-03-16T08:35:25Z

Hello @Axel-Naumann, @pcanal, are the failed test related to my patch?
Is there something else you want me to change, otherwise could you accept it?

Axel-Naumann · 2020-03-16T09:06:09Z

are the failed test related to my patch?

No, sorry about that - please ignore them.

Is there something else you want me to change, otherwise could you accept it?

I'd like @pcanal to have the final say here.

pcanal · 2020-03-16T14:54:23Z

@cosimoNigro thank you for the PR :)

cosimoNigro added 3 commits March 5, 2020 18:01

made possible to read and print variable-length array from FITS columns

86e0ecc

return TArrayD with variable-length array in Cell

5c0e4d3

added tutorial and example of FITS file with variable-length array in…

0dfe78a

… columns

cosimoNigro requested a review from couet as a code owner March 7, 2020 11:47

cosimoNigro commented Mar 7, 2020

View reviewed changes

cosimoNigro changed the title ~~Reading FITS table columns embedding variable-lengths arrays~~ Reading FITS table columns embedding variable-length arrays Mar 7, 2020

cosimoNigro mentioned this pull request Mar 9, 2020

Defining input formats for ROOT and FITS data javierrico/gLike#25

Closed

Axel-Naumann requested a review from pcanal March 9, 2020 13:37

pcanal reviewed Mar 9, 2020

View reviewed changes

cosimoNigro mentioned this pull request Mar 9, 2020

Including one of the rmf spectral files in the ROOT repository open-gamma-ray-astro/joint-crab#7

Closed

pcanal reviewed Mar 9, 2020

View reviewed changes

added new line ending FITS_tutorial8.C, replaced repeated offset var …

e8eb4f4

…in TFITS.cxx

eguiraud assigned pcanal Mar 10, 2020

cosimoNigro added 2 commits March 10, 2020 10:46

changed name of correct variable in TFITS.cxx

dbcfbb4

improved nomenclature in TFITS.cxx

034c8b5

added gROOT->GetTutorialDir() in FITS_tutorial8.C

63423ee

pcanal reviewed Mar 10, 2020

View reviewed changes

changed the array passed to fits_read_descript to a vector

3287182

changed input test files to rmf from sherpa test data

479af99

pcanal merged commit 9134d8b into root-project:master Mar 16, 2020

Reading FITS table columns embedding variable-length arrays #5099

Reading FITS table columns embedding variable-length arrays #5099

Uh oh!

Conversation

cosimoNigro commented Mar 7, 2020

Uh oh!

phsft-bot commented Mar 7, 2020

Uh oh!

cosimoNigro Mar 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Axel-Naumann commented Mar 9, 2020

Uh oh!

phsft-bot commented Mar 9, 2020

Uh oh!

phsft-bot commented Mar 9, 2020

Warnings:

Uh oh!

phsft-bot commented Mar 9, 2020

Warnings:

Uh oh!

phsft-bot commented Mar 9, 2020

Warnings:

Uh oh!

phsft-bot commented Mar 9, 2020

Warnings:

Uh oh!

phsft-bot commented Mar 9, 2020

Warnings:

Failing tests:

Uh oh!

pcanal Mar 9, 2020

Choose a reason for hiding this comment

Uh oh!

cosimoNigro Mar 10, 2020

Choose a reason for hiding this comment

Uh oh!

phsft-bot commented Mar 9, 2020

Uh oh!

phsft-bot commented Mar 9, 2020

Warnings:

Uh oh!

pcanal Mar 9, 2020

Choose a reason for hiding this comment

Uh oh!

cosimoNigro Mar 10, 2020

Choose a reason for hiding this comment

Uh oh!

pcanal commented Mar 9, 2020

Uh oh!

cosimoNigro commented Mar 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pcanal commented Mar 10, 2020

Uh oh!

pcanal commented Mar 10, 2020

Uh oh!

phsft-bot commented Mar 10, 2020

Uh oh!

phsft-bot commented Mar 10, 2020

Errors:

Failing tests:

Uh oh!

pcanal commented Mar 10, 2020

Uh oh!

cosimoNigro commented Mar 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pcanal commented Mar 10, 2020

Uh oh!

phsft-bot commented Mar 10, 2020

Uh oh!

phsft-bot commented Mar 10, 2020

Errors:

Failing tests:

Uh oh!

phsft-bot commented Mar 10, 2020

Errors:

Failing tests:

cosimoNigro Mar 7, 2020 •

edited

Loading

cosimoNigro commented Mar 10, 2020 •

edited

Loading

cosimoNigro commented Mar 10, 2020 •

edited

Loading

cosimoNigro Mar 11, 2020 •

edited

Loading

cosimoNigro commented Mar 12, 2020 •

edited

Loading