Allows DataStoreMaker to be used with IRFs not following CALDB structure

[QRemy]

DataStore.from_events_files is used to create HDU and observation index tables from the EVENTS header. But this works only for IRFs following the CALDB structure, and the events file header have to contains the TELESCOP, CALDB, and IRF keywords (not part of GADF).

This PR improve the generation of the irf filename from the CALDB infos, and alternatively allow to read it directly from an extra keyword IRF_FILE in the events metadata.
Now, the events_sampling notebook show how to add this keyword, so the index files generated are not broken.

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[adonath]

Related: open-gamma-ray-astro/gamma-astro-data-formats#183

[adonath]

Conclusion from the dev meeting:

• Adding new keywords requires changes and discussion in GADF (see link above)
• We can introduce Observation.write() to write events and IRF into one file
• We can introduce a helper function like create_hdu_index_table(observations, irf_filenames={}) to create the HDU index table for the case where only one single set of IRFs is used

[QRemy]

I modified the DataStoreMaker to accept an irf_paths optional argument,
so now we can call DataStore.from_events_file(events_paths, irf_paths)
If irfs_paths is provided it must be the same length than events_paths (shown in the notebook).
If None the events file header have to contain CALDB and IRF keywords to locate the IRF file,
otherwise the IRFs are assumed to be contained in the events files.
This should cover all the use cases we have.

[adonath]

@QRemy Maybe still support the case where irf_path is a single dict internally and document it? Just to make it even more convenient for users?

[QRemy]

@QRemy Maybe still support the case where irf_path is a single dict internally and document it? Just to make it even more convenient for users?

As the irfs argument in Observation.create ?

[adonath]

No, I meant something along the lines of:

def from_events_files(event_paths, irf_paths):
    """..."""
    if len(irf_paths) == 1:
        irf_paths = [irf_paths] * len(event_paths)

[codecov]

Codecov Report

Merging #3846 (81f6a80) into master (fd0bb96) will increase coverage by 0.03%.
The diff coverage is 95.00%.

@@            Coverage Diff             @@
##           master    #3846      +/-   ##
==========================================
+ Coverage   93.77%   93.81%   +0.03%     
==========================================
  Files         162      162              
  Lines       20044    20060      +16     
==========================================
+ Hits        18797    18819      +22     
+ Misses       1247     1241       -6     

Impacted Files	Coverage Δ
gammapy/data/data_store.py	92.80% <94.59%> (+1.89%)	⬆️
gammapy/utils/scripts.py	95.65% <100.00%> (+0.19%)	⬆️
gammapy/modeling/iminuit.py	94.39% <0.00%> (+1.86%)	⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[registerrier]

Thanks @QRemy .

I have left some inline comments in the notebook.

@fabiopintore is this new behavior fine?

[registerrier]

Is it useful here? It is only when reading that you need the path to the irf now, no?

[QRemy]

In general observations can have different irf files so the list of irfs_path have to be defined before the loop. I wanted to stress that in the notebook, even if it is not necessary for this use case (but we could use 2 different zenith angles for example).

[registerrier]

This is probably where you could define the list of irf_path no?

[fabiopintore]

It seems OK to me and no further comments from my side. Thanks @QRemy to have taken care of this!

[registerrier]

Thanks @QRemy . This looks good to me.