esheldon / fitsio

A python package for FITS input/output wrapping cfitsio
GNU General Public License v2.0
133 stars 57 forks source link

A start on supporting Logical columns #6

Closed dstndstn closed 10 years ago

dstndstn commented 11 years ago

Hi,

I just ran across the issue that logical (boolean) columns in bintables don't seem to be supported. The following diff starts to address that -- it's good enough for my purposes, but doesn't cover all code paths.

diff --git a/fitsio/fitsio_pywrap.c b/fitsio/fitsio_pywrap.c
index 7beac58..fe9625a 100644
--- a/fitsio/fitsio_pywrap.c
+++ b/fitsio/fitsio_pywrap.c
@@ -614,6 +614,8 @@ npy_to_fits_table_type(int npy_dtype) {

     char mess[255];
     switch (npy_dtype) {
+        case NPY_BOOL:
+            return TLOGICAL;
         case NPY_UINT8:
             return TBYTE;
         case NPY_INT8:
@@ -2253,6 +2256,21 @@ PyFITSObject_read_column(struct PyFITSObject* self, PyObject* args) {
             set_ioerr_string_from_status(status);
             return NULL;
         }
+
+       /*  I thought I was hitting this code path, but nope; 10% chance this works
+       if (PyArray_ISBOOL(array)) {
+         printf("Boolean array -- converting data\n");
+         // cfitsio reads as char 'T'/'F' -- convert data array to 0/1.
+         npy_intp i;
+         npy_int8* bdata = (npy_int8*)data;
+         for (i=0; i<nrows; i++) {
+           printf("0x%x ", bdata[stride*i]);
+           bdata[stride * i] = ((char)(bdata[stride * i]) == 'T') ? 1 : 0;
+         }
+         printf("\n");
+       }
+       */
+         
     }
     Py_RETURN_NONE;
 }
diff --git a/fitsio/fitslib.py b/fitsio/fitslib.py
index 743029c..6b8cdfd 100644
--- a/fitsio/fitslib.py
+++ b/fitsio/fitslib.py
@@ -1776,6 +1779,10 @@ class FITSHDU:
                 self._rescale_array(array[name], 
                                     self._info['colinfo'][colnum]['tscale'], 
                                     self._info['colinfo'][colnum]['tzero'])
+                # cfitsio reads as characters 'T' and 'F' -- convert to real boolean
+                if array[name].dtype == numpy.bool:
+                    array[name] = (array[name].astype(numpy.int8) == ord('T'))
+                   
         lower=keys.get('lower',False)
         upper=keys.get('upper',False)
         if self.lower or lower:
@@ -3623,7 +3630,7 @@ _hdu_type_map = {IMAGE_HDU:'IMAGE_HDU',
 # no support yet for complex
 _table_fits2npy = {11:'u1',
                    12: 'i1',
-                   14: 'i1', # logical. Note pyfits uses this for i1, cfitsio casts to char*
+                   14: 'b1', # logical. Note pyfits uses this for i1, cfitsio casts to char*
                    16: 'S',
                    20: 'u2',
                    21: 'i2',
@@ -3646,6 +3653,7 @@ _table_fits2npy_ascii = {16: 'S',

 # for TFORM
 _table_npy2fits_form = {'u1':'B',
+                        'b1':'L',
                         'i1':'S', # gets converted to unsigned
                         'S' :'A',
                         'u2':'U', # gets converted to signed
dstndstn commented 11 years ago

This allows writing and then reading back a table with a boolean column.

esheldon commented 11 years ago

This is really great Dustin. This has been in the todo list for a long time. Do you have a fork I can pull from?

dstndstn commented 11 years ago

It only covers one code branch though -- and may break the other code branch! The main issue seems to be around logical columns being written as 'T' / 'F' -- or is it just cfitsio returning that? -- and I worked around that by, in the python code, creating a numpy boolean array, passing that to cfitsio as usual, and then post-processing it in python. The main issue is that if you use the boolean array and don't post-process it, then it contains 'T' or 'F', BOTH of which evaluate to TRUE. So it appears to work but returns the wrong answer -- BAD NEWS! I'm afraid I'm in a rush right now so I don't have time to unit test and make sure the other code branches work. Can I ask you to ping me on this in a week if you don't hear anything by then?

esheldon commented 10 years ago

closing since this was a dup