Here is my report for the Science Tools checkout.

I used version v5r1p4 (binary) from the installer.
I have played with the applications of the map_tools package.
I have judged them against my expectation of what they should do
(not against a precise set of predefined requirements).
The rule of the game was to go as far as I could by myself (no
contact with the developer).
I have used only the documentation (not looked into the code).
In other words, I tried to put the boots of an 'ordinary user' on.

Cheers,
Jean

----------------------------------------------------------------------

General comments:
-----------------

1. Documentation
----------------
The way the documentation prepared by doxygen is presented is
particularly awkward for the users. It looks much more like a developer's
documentation. First I could not find a general entry point for the
Science Tools which would give access to general information and list
(in an organized way) the available tasks.
Second, within the packages I looked at (mostly map_tools and
Likelihood), the 'real' doc for the user is found under the 'Related
pages' item, really not the place where one would look first.
I think the point of view should be entirely inverted. The user's
documentation should come first, and the developer's doc second.
In particular, the user does not care about the C++ classes.

2. General settings
-------------------

- Scripts
The definition of INST_DIR in the scripts which call the tasks
(and define all environment variables) is wrong. The line
"INST_DIR=`dirname $execScript/ | xargs dirname | xargs dirname`"
should read "INST_DIR=`dirname $execScript/ | xargs dirname`"
(only the bin subdirectory must be removed).

- Parameter interface
The parameter interface does not handle gracefully wrong input.
For example, if I make a typo in one parameter name, I get the error
message 'Caught N5hoops12PILExceptionE at the top level: Could not open
parameter file for exposure_map (at ../src/hoops_pil.cxx: 371)'.
This is not very useful. It should write 'parameter soandso does not
exist' so that the user has a chance to understand his mistake.

- Verbosity
I did not understand how I could choose the verbosity of the
tasks. Does this have anything to do with the 'chatter' parameter ?
This doesn't seem to have any effect.


Comments specific to map_tools:
-------------------------------

1. Documentation
----------------

- The description of what the tasks do is very skimpy. It should contain
at least:
* a description of the context (to what aim do you use those tasks) and
how they articulate with the rest of the software.
* a description of the input/output files. This can be just a reference
when the files are defined elsewhere (like the FT1/FT2 files), but
must be more than that for the other files.

- Environment variables
The doc says "The package requirements file defines the following
environment variables ..." To me, this is developer's jargon. What the
doc should say is that the user should define those environment
variables.
As far as I know, the PFILES variable is required by all packages, so I
am not sure why it is mentioned explicitly here.
It looks like INFILES is not used at all. I argue below that OUTFILES
is not useful. Then this section could be removed entirely.


2. count_map
------------

- There exist two other applications (gtcntsmap under the Likelihood
package and evtbin) which do very similar things. This should be
consolidated into a single application which would cumulate the
functionalities.
- The documentation mentions the 'ftselect' application for selecting
rows. Is that a typo ? Should it read 'fselect' ?
- The parameter file should distinguish between optional and mandatory
parameters. Currently all parameters are mandatory.
- The extension name must be specified explicitly. This is not very
user friendly. Normally the extension to be considered will always be
'EVENTS'. This should be default (with the option to specify the name
if the default is not suitable).
- The default for ra_name/dec_name should be RA/DEC (FT1 default).
- The map is written to the 1st extension of the output file. This kind
of data (n-dimensional array) is more logically written to the
'Primary'. Also the map is formally a 3D array (nx * ny * 1).
It should be a simple 2D array.
- Many projections (certainly ARC, SIN, AIT) do not work on a subset
of the sky. One gets strange aliases as if a bigger image was folded
around the borders and stacked onto the central image.


3. exposure_cube
----------------

- There exists a makeExposureCube application under the Likelihood
package which apparently does the same thing (with a slight difference
in the way the time selection is passed). There should be a single
application to build the exposure cube.
- The parameter file should distinguish between optional and mandatory
parameters. Currently all parameters (including the input file !) are
optional.
- I do not like very much the idea of sending the output file by default
to the /tmp directory. The FTOOLS standard is that the default output
file appears in the current directory.
- The costhetabinsize parameter is not respected. If the expected number
of bins between 0 and 1 is nbins = 1/costhetabinsize, the real number
of bins in output is nbins-1 (so that costhetabinsize is larger than
required). It looks like 1/costhetabinsize is somehow rounded.
I think the task should use costhetabinsize as entered, rather than
force the costheta interval to be exactly [0,1].
- Strangely enough, the task does not do something like that (rounding up
the parameter value) for pixelsize, where it would be really justified
(to cover the entire sky).
- The third dimension is actually sqrt(1-costheta) rather than costheta
directly. I do not understand this choice, if indeed the effective area
is nearly linear in cos(theta). In addition, since cos(theta) depends
linearly on the solid angle, building the histogram on cos(theta)
directly results in a much flatter histogram.
- I could not make the filter and uselb parameters to do anything.
They appear to be ignored by the task.
The output was always in RA/Dec coordinates.
- The projtype parameter is inoperant (this is just as well) and
should be dropped. I did not test the rot parameter, but I am not sure
why anyone would want the sky representation to be rotated either.
- Other selections than just on time should be possible (like on zenith
angle). Maybe this is what the filter parameter is for ?
It should be possible to enter those selections via a map
(or an events list) obtained after some filtering (so that the cube
will be coherent with the map).
Another important selection which must be taken into account is the
Good Time Intervals (for example, the missing run in the 1st day of
DC1).
- The cube is written to the 1st extension of the output file. This kind
of data (n-dimensional array) is more logically written to the
'Primary'.
- The CTYPE1 keyword in the hypercube file is incorrect. It should be
set to 'RA---CAR' (with three '-', not 'RA--CAR').
- The CRPIX3 keyword appears incorrect. It is set (for costhetabinsize
default) to 20. This means that inclination angle 0 should correspond
to pixel 20 in the 3rd direction. It is clearly not true from the
contents of the cube (inclination angle 0 corresponds to the 1st pixel
in the 3rd direction). I believe CRPIX3 should be set to 0.5.
- The spatial representation of the sky is inverted. An easy way to see
it is to look at the 1st map (for inclination angle 0) with fv.
The first values (at small time) of RA_SCZ/DEC_SCZ are 11.06/-6.51,
whereas the map shows non-zero values at -11.06/+6.51.
- I tried to understand the (small) differences between the results of
exposure_cube (after spatial inversion) and of my IDL program.
The time granularity of exposure_cube appears to be 60 s (as in FT2),
so that no spatial interpolation is done. I suspect this is not precise
enough (the distance between two successive pointing directions in
the FT2 file is > 3 degrees). I personally interpolate to 1 s.
- The routine could be sped up a bit. On the 6 days, exposure_cube
takes 7.5 mn on my PC. My IDL routine takes 4 mn (with the time
interpolation).


4. exposure_map
---------------

- The parameter file should distinguish between optional and mandatory
parameters. Currently all parameters (including the input file !) are
optional.
- The effective area is hard-wired into the code. This means that the
exposure map is not consistent with what obsSim does. Since the doc
does not say precisely what the task does, even the current
functionality could not be tested.
- Other selections than just on energy should be possible (like on
inclination angle). It should be possible to enter those selections via
a map (or an events list) obtained after some filtering (so that the
exposure map will be coherent with the map).
- It should be possible to define the pixel size, the dimensions and the
centering via an input map as well (for the same reason).
- I do not like very much the idea of sending the output file by default
to a directory defined by the OUTFILES environment variable. The FTOOLS
standard is that the default output file appears in the current
directory.
- The map is written to the 1st extension of the output file. This kind
of data (n-dimensional array) is more logically written to the
'Primary'. Also the map is formally a 3D array (nx * ny * 1). It should
be a simple 2D array.
- The application is not robust to wrong input.
For example, projtype=WWW results in a segmentation fault.
npix=-1 results in 'Caught St9bad_alloc at the top level: St9bad_alloc'.
- Contrary to the hypercube, the exposure map does not appear inverted.


5. read_map
-----------

- This application does two very different things at the same time.
It reads a map and writes the value at a particular point to STDOUT,
and it reprojects the map onto any other projection.
This second functionality can be very useful, but it should be made
separate (I suggest to call it 'reproject').
- For the simple read_map (just the answer written to STDOUT), it would
be useful to keep uselb (and make it do something).
- The parameter file should distinguish between optional and mandatory
parameters. Currently all parameters are mandatory.
- The application requests a 'table_name' parameter. If I add it to the
parameter file, it looks like it works.
- On a normal image, I got the error 'attribute NAXIS3 not found'.
Why should an image have an NAXIS3 keyword ?
- On an exposure map (output of exposure_map), it works if I specify
table_name explicitly (not very user friendly).
- On an exposure cube (output of exposure_cube), it does something but I
wonder what (possibly read the 1st map, for inclination angle=0).
- When reprojecting a RA/Dec map in AITOFF projection onto l/b (also
AITOFF), a line of very large values appears (corresponding to the
borders of the RA/Dec map at RA=12:00).
- When reprojecting an l/b map in CAR projection onto RA/Dec (also CAR),
a few NaN appear at the pole of the original map (near b=+90).