Response and GBM files

[grburgess]

There seems to be an issue with the reading of GBM DRMs (similar to what I have seen with gammapy)

/usr/local/Cellar/python/HEAD/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/threeML-0.3.2-py2.7.egg/threeML/plugins/ogip.pyc in _read_matrix(self, data, n_channels)
    750             for j in range(n_grp[i]):
    751 
--> 752                 rsp[i, f_chan[i][j]: f_chan[i][j] + n_chan[i][j]] = matrix[i][m_start:m_start + n_chan[i][j]]
    753 
    754                 m_start += n_chan[i][j]

ValueError: could not broadcast input array from shape (128) into shape (127)

[grburgess]

I believe the issue lies in FOTRAN style indexing:

f_chan[i][j] = 1 implies that the first channel is the first entry (at least in GBM DRMs)
but the array indexing for Numpy would want this to be zero. you 1:128 is a 127 length array when you really want a 128 length array.

Is this general or GBM specific?

[grburgess]

Ok, so I tried the new plugin on XRT DRMs. It worked fine and they correctly specify the first channel as zero. GBM specifies the first channel as 1. I propose making and exception when there is a broadcast error where FCN has minus one removed. In fact, in my other code I was always doing this. I can make the change and commit. @giacomov , thoughts?

[giacomov]

I have already corrected this a couple fo days ago and tested on a GBM matrix, probably there are different cases. Can you post somewhere the matrix which doesn't work?

This part is a little tricky, because if you get it wrong you can bias your spectral results in a non-obvious way, so we need to be careful and really understand what is going on.

[grburgess]

Here is an RSP (from the example folder) it does not natively work.

This change causes it to work:

        #
                try:

                    rsp[i, f_chan[i][j]: f_chan[i][j] + n_chan[i][j]] = matrix[i][m_start:m_start + n_chan[i][j]]

                except(ValueError):

                    rsp[i, f_chan[i][j] -1: f_chan[i][j]-1 + n_chan[i][j]] = matrix[i][m_start:m_start + n_chan[i][j]]

glg_cspec_n3_bn110731465_v00.rsp2.zip

[giacomov]

@drJfunk I have fixed this already in the new OGIPLike plugin.

When you have time, please port the TTE plugin to the new OGIP plugin, by making it inherit from there instead of the old GenericOGIPLike plugin, and update the example as needed.

I already updated the basic_example.ipynb notebook for your reference.

The new plugin solves a lot of issues of the old one, and it is much cleaner and short.

[giacomov]

yeah, I wanted to discuss with you what you would like to have in the parent class (OGIPLike) and what is instead specific to the GBM.

The plan is to retire GBMLike (or make it just another name of OGIPLike) as it is really confused and bug-prone.

The new class features different statistics which are selectable (although the plugin decide by itself what is appropriate depending on the POISSERR keywords in the files).

It is also way less confused.

At the moment there is only one method that we should definitely port, which is view_counts_spectrum.

Anything else which is important for any PHA spectrum, so it should be in the parent class?

[grburgess]

I just realized what has happened and am taking a look at the TTElike. For the moment, I see the TTElike being similar to Veritas or HAWC or LAT because it has some additional functionality that is specific to the data type. I think TTELike can be the best GBM plugin because it enables more flexibility and just keeping an OGIPlike as an option is fine. I like some of the gammapy response functions because they allow users to explore the DRMs which can be useful. Besides that, I think OGIPLike is flexible enough on its own (the data prep is always done outside the frame work). People can build more advanced plugins that do the data prep and ultimately pass through to OGIPLike in the end. Does it make sense?

[grburgess]

However, the OGIPLike constructor isn't suitable for the TTE data. It will need to be virtualized, or the constructor will need to have the ability to customize the file type outside of PHA. We had the same problem originally, what path do you want to take @giacomov ?

[giacomov]

I think the most sensitive would be to add a static method like "from_counts" to the OGIPLike class, so essentially you can build an instance on the fly without having to go through a .pha and a .bak file.

Would this be sufficient?

[grburgess]

Yes. I think I get the idea. Not to convolute the issue, but perhaps an EVENT class is warranted? This could be a solution that is general enough for other similar data types?
/J. Michael

Sent my iPhone

On Sep 3, 2016, at 15:41, Giacomo Vianello notifications@github.com wrote:

I think the most sensitive would be to add a static method like "from_counts" to the OGIPLike class, so essentially you can build an instance on the fly without having to go through a .pha and a .bak file.

Would this be sufficient?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[giacomov]

I like it. An EventList class which provides a get_pha method to get a PHA on the fly (as well as other things).

Are you going to work on this, or should I?

[grburgess]

I can grab it. Have you a suggestion of another data type outside of GBM tte that I can look at so that I don't make it too GBM specific?

/J. Michael

Sent my iPhone

On Sep 3, 2016, at 16:05, Giacomo Vianello notifications@github.com wrote:

I like it. An EventList class which provides a get_pha method to get a PHA on the fly.

Are you going to work on this, or should I?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[giacomov]

I'd make a constructor which gets simple numpy arrays, and then you can make static methods like from_TTE to specialize to specific file formats (like pandas does with DataFrame and other containers).

Each mission is going to need a reader for their specific format, and at some point we will also need to support the universal format that the gammapy people are defining.

[grburgess]

Cool. This leads into my next question which is related to your early comments.

Will we go for Like plugins or use the more general instance? If there is a static method that takes TTE it means that at some point there will need to be modifications for future instruments. Perhaps it is better to use an inheritance scheme

/J. Michael

Sent my iPhone

On Sep 3, 2016, at 16:13, Giacomo Vianello notifications@github.com wrote:

I'd make a constructor which gets simple numpy arrays, and then you can make static methods like from_TTE to specialize to specific file formats (like pandas does with DataFrame and other containers).

Each mission is going to need a reader for their specific format, and at some point we will also need to support the universal format that the gammapy people are defining.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[grburgess]

Well, it turns out that GBM does not use the TLMIN keyword (at least not universally):

/usr/local/Cellar/python/HEAD/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/astropy/io/fits/header.pyc in _cardindex(self, key)
   1727 
   1728         if not indices:
-> 1729             raise KeyError("Keyword %r not found." % keyword)
   1730 
   1731         try:

KeyError: "Keyword 'TLMIN4' not found."

Therefore, the response code cannot read the file.
glg_cspec_n3_bn080916009_v07.rsp.zip

[grburgess]

I've added a way around this by assuming TLMIN = 1 when it cannot find it and will be committed with this entire update