ctools

I started working on the code. While adapting the get_parameters()-function of each tool, I was wondering about the purpose of ctskymap. To me, it seems like a very special case of ctbin (just using one energy bin). Am I right?

I worked further on the ctools-base class and successively adapted the individual ctools-classes. The changes I made are available on branch 1360-extend-ctools-base-class. I only required minor changes on the gammalib-level. I’ve added a new constructor to GCTAExposure and GCTAMeanPsf. The corresponding branch on gammalib is 1360-extend-ctools-base-class-gammalib.
Warning: I changed a lot of the code. Every get_parameters()-function has been adapted to use methods which were implemented in the base class. In addition, I changed the parameter names following the proposal on https://cta-redmine.irap.omp.eu/boards/14/topics/209

• GObservations get_obs(bool get_response): It builds the observation container depending on user parameters. Currently there are four possibilities:
  • pass an observation container on the tool constructor
  • provide an XML observation definition file as parameter "inobs”
  • provide an FITS observation file (event list or counts cube) as parameter "inobs”
  • use inobs="NONE", inducing a query for parameters to build an observation from scratch
• GCTAEventCube get_cube(): builds a cta event cube object from user parameters. In case a GObservations-object is passed, this function uses the pointing position instead of xref and yref values.
• GSkyDir get_pointing(const Observations& obs). Is called in the above function to find the pointing position from an observation container. In case several observations are in the container, a simple averaging is applied to find the mean pointing
• GSkymap get_map(): Similar as the get_cube() - function. However without querying for the GEbounds.
• GCTAEventCube set_from_cntmap(const std::string filename);: Loads an event cube definition from file.

Each tool has the “freedom” to call these functions as required. By outsourcing some of these functions, I managed to get rid of many protected members from the individual tools.
The current version is working when using the script test_ctools.sh. However, make check fails due to an import error in the python test suite (which I don’t quite undertstand):

Test cspull: 
Traceback (most recent call last):
  File "/Users/mimayer/Software/ctools/scripts/cspull.py", line 22, in <module>
    import ctools
  File "/Users/mimayer/Software/ctools/pyext/ctools/__init__.py", line 3, in <module>
    import obsutils
  File "/Users/mimayer/Software/ctools/pyext/ctools/obsutils.py", line 22, in <module>
    import ctools
  File "/Users/mimayer/Software/ctools/pyext/ctools/ctools.py", line 26, in <module>
    _ctools = swig_import_helper()
  File "/Users/mimayer/Software/ctools/pyext/ctools/ctools.py", line 18, in swig_import_helper
    import _ctools
ImportError: No module named _ctools

I hope I did not break anything by changing so much of the code. However, the parameter interface seems now more homogenous throughout the tools.

What do you think?

I went through the code. Here some comments in the order I checked the code:

I recognised that you added an event cube constructor to GCTAExposure that copies exposure information from an event cube. I added a statement that sets all pixel values to zero as a constructor should provide an object in a well defined clean state. For GCTAMeanPsf I change the constructor in the same way by using a newly coded GSkymap::nmaps method that allows changing the number of maps in a sky map object. I pushed the modification in the 1360-extend-ctools-base-class-gammalib branch.

I could not reproduce your cspull import error. make check works on my side without any problems. Have you make a make clean before building? Maybe you have still some old code around?

I see some stuff in cspull that looks like debugging code. Same for cstsdist. I did remove this stuff. Unit tests still work.

Concerning the ctool::set_from_cntmap method I was wondering whether the WCS check was needed? It would be good if the code could - at least in principle - apply to any kind of projections. I see no reason why this should not work. In any case, I added a filename constructor to GCTAEventCube which makes this method basically obsolete.

I renamed ctool::get_pointing to ctool::get_mean_pointing to emphasize that the method does some averaging. However, the method needs to be improved to take into account wrap arounds in Right Ascension and to handle also coordinates near the celestial poles.

I changed the code in ctool::get_cube to reuse the ctool::get_map methods.

Finally, I still see some code repetition among the different tools (for example the get_cube and get_map methods that need two instances that are always used the same way). I modified the ctool base class so that the usepnt parameter is handled at that level, hence only a single instance of the method is used. I furthermore renamed the methods to create_cube and create_map as the get term could be mis-interpreted as loading a cube from a file (get a cube from somewhere). I propose to reserve the get methods for methods that get for example some data from a file.

• get_ebounds to create_ebounds
• setup_obs to create_cta_obs

I furthermore added a require_inobs method to throw an exception in case that the inobs parameter is NONE or empty. This further reduces redundant code in many of the tools.

I pushed the code into 1360-extend-ctools-base-class.

I have not yet done tests of the tools beyond the unit tests. In particular, I have not yet checked the results of the tools (i.e. if they still operate as expected and produce the results as expected). As this is a major change, we should make sure that no problems occur before we merge the code into the trunk.

Thanks for the extensive feedback.

I recognised that you added an event cube constructor to GCTAExposure that copies exposure information from an event cube. I added a statement that sets all pixel values to zero as a constructor should provide an object in a well defined clean state. For GCTAMeanPsf I change the constructor in the same way by using a newly coded GSkymap::nmaps method that allows changing the number of maps in a sky map object. I pushed the modification in the 1360-extend-ctools-base-class-gammalib branch.

Perfect, of course the constructor should give a clean object. Thanks for catching this.

I could not reproduce your cspull import error. make check works on my side without any problems. Have you make a make clean before building? Maybe you have still some old code around?

A clean checkout of gammalib and ctools solved this problem. I guess there were some version confusions.

I see some stuff in cspull that looks like debugging code. Same for cstsdist. I did remove this stuff. Unit tests still work.

Probably same as above, due to a commit error. Thanks for cleaning this up.

Concerning the ctool::set_from_cntmap method I was wondering whether the WCS check was needed? It would be good if the code could - at least in principle - apply to any kind of projections. I see no reason why this should not work. In any case, I added a filename constructor to GCTAEventCube which makes this method basically obsolete.

Agreed, this is a much better approach.

I renamed ctool::get_pointing to ctool::get_mean_pointing to emphasize that the method does some averaging. However, the method needs to be improved to take into account wrap arounds in Right Ascension and to handle also coordinates near the celestial poles.

This is very true. One has to think about a proper way how to average the given coordinates. Ultimately, we might need this functionality anyway if we go to pointing tables. The potential GCTAPointings class (#1176) could return its average direction.

I changed the code in ctool::get_cube to reuse the ctool::get_map methods.

great!

Finally, I still see some code repetition among the different tools (for example the get_cube and get_map methods that need two instances that are always used the same way). I modified the ctool base class so that the usepnt parameter is handled at that level, hence only a single instance of the method is used. I furthermore renamed the methods to create_cube and create_map as the get term could be mis-interpreted as loading a cube from a file (get a cube from somewhere). I propose to reserve the get methods for methods that get for example some data from a file.

I also agree to the name changes, which make the thing more clear.

I have not yet done tests of the tools beyond the unit tests. In particular, I have not yet checked the results of the tools (i.e. if they still operate as expected and produce the results as expected). As this is a major change, we should make sure that no problems occur before we merge the code into the trunk.

I went over the code and did not find any obvious problems. The test cases work for me as well. I will conduct some more sophisticated analyses to see if we encounter problems somewhere. I will provide further feedback very soon.

Another thing:
The cscripts are currently not affected by this change. However, it would be great to be able to use the same functionality from these python scripts to setup the observations, maps, etc.. I did not find a way to call protected functions from the ctool-base class via python. Would it therefore make sense to make all the new methods public and add them to the ctool.i-file. Or do you have a better idea?

ok great. Thanks for the quick solution. I created an issue to adapt the cscripts accordingly (#1408).

In the meantime, I checked all individual tools carefully. In general, everything went fine. Binned and unbinned results looked reasonable. However, I did of course not check every possibility of parameter combinations.

1. the .par file from ctpsfcube has a copy&paste error from ctexpcube (it just asks for output exposure cube file)
2. in ctbkgcube::run() (line 266), there is a check whether the background model is valid. However, this function checks for validity with the identifier "", which always gives me false in case I provide a proper id. I therefore changed the code that it only checks for the possible instrument names (“CTA",“HESS",“MAGIC” or “VERITAS”).

I pushed these changes to the working branch. From my side there is green light to proceed to merge the changes. Are there any additional tests you want to be performed first?

Merged into devel.

This one could lead to problems when a model applies to several instruments, as the instruments() method then returns, e.g., “CTA,HESS”.

Very true. Haven’t thought of this possibility.

I thus reverted the change but made a modification to GModel::is_valid() that ignores a check if a blank string is provided.

Great.

Merged into devel.

Thanks.