gammasim / simtools

Tools and applications for the Simulation System of the CTA Observatory.
https://gammasim.github.io/simtools
BSD 3-Clause "New" or "Revised" License
10 stars 1 forks source link

Integration of telescope_layout tools into gammasim-tools #329

Closed GernotMaier closed 1 year ago

GernotMaier commented 2 years ago

Merge telescope_layout repository into gammasim-tools. This avoids duplication of code. Archive the telescope_layouts repository after the successful integration into gammasim-tools.

Following applications will be included into gammasim-tools:

  1. print_layout.py (rename to print_array_elements.py): prints coordinates of array elements (in most cases telescope positions) in the different used coordinate systems
  2. compare_layouts.py (rename to compare_array_elements.py): compare the positions of two lists of array elements and print differences

Integrate updates in telescope_layout code:

Additional functionality:

Suggest not to include the plotting in this issue (see also Sites branch), but do this as a next step.

GernotMaier commented 2 years ago

Some points to be discussed.

simtools/layout/telescope_position.py:

GernotMaier commented 2 years ago

A couple discussion points, related to coordinate systems and telescope positions (see also draft PR #330)

1. Telescope altitude.

Decided to use the altitude (asl) as 'z' position for all coordinate system, including the corsika system. The logic is that the telescope position should be independent of the telescope type (i.e., sphere radius).

To write out the coordinates in corsika coordinates, one out use in the TelescopePosition class the function

corsikaZ = convertTelescopeAltitudeToCorsikaSystem(telAltitude, corsikaObsLevel, corsikaSphereCenter)

If we agree on that, then I go carefully again through the code to ensure that this is propagated correctly.

2. CORSIKA sphere center and radius

The sphere centre is required to calculated the CORSIKA Z position and is currently hardwired in the code:

   @staticmethod
    def _corsikaTelescopeDefault():
        """
        Default values for CORSIKA telescope dict

        Returns
        -------
        dict
            corsika telescope default values

        TODO: discuss hardwired corsika parameters

        """
        return {
            "corsika_obs_level": None,
            "corsika_sphere_center": {
                "LST": 16.0 * u.m,
                "MST": 9.0 * u.m,
                "SST": 3.25 * u.m,
            },
            "corsika_sphere_radius": {
                "LST": 12.5 * u.m,
                "MST": 9.6 * u.m,
                "SST": 3.5 * u.m,
            },
        }

This also requires a function to derive the telescope type/class from the telescope name.

Could we get rid of this? Is this information in the data base? If yes, could you help me on how to get it?

The sphere radius is required for the writing of the CORSIKA telescope configuration, I guess it is also in the DB?

orelgueta commented 2 years ago

1. Telescope altitude.

Decided to use the altitude (asl) as 'z' position for all coordinate system, including the corsika system. The logic is that the telescope position should be independent of the telescope type (i.e., sphere radius).

To write out the coordinates in corsika coordinates, one out use in the TelescopePosition class the function

corsikaZ = convertTelescopeAltitudeToCorsikaSystem(telAltitude, corsikaObsLevel, corsikaSphereCenter)

If we agree on that, then I go carefully again through the code to ensure that this is propagated correctly.

The concept is perfectly fine, but why would the user ever need just the altitude? Why is it not "convert to CORSIKA" coordinates which takes care of everything all at once?

   @staticmethod
    def _corsikaTelescopeDefault():
        """
        Default values for CORSIKA telescope dict

        Returns
        -------
        dict
            corsika telescope default values

        TODO: discuss hardwired corsika parameters

        """
        return {
            "corsika_obs_level": None,
            "corsika_sphere_center": {
                "LST": 16.0 * u.m,
                "MST": 9.0 * u.m,
                "SST": 3.25 * u.m,
            },
            "corsika_sphere_radius": {
                "LST": 12.5 * u.m,
                "MST": 9.6 * u.m,
                "SST": 3.5 * u.m,
            },
        }

This also requires a function to derive the telescope type/class from the telescope name.

Could we get rid of this? Is this information in the data base? If yes, could you help me on how to get it?

The sphere radius is required for the writing of the CORSIKA telescope configuration, I guess it is also in the DB?

The sphere radius (and more generally any CORSIKA parameters) are not in the DB. A few weeks ago we discussed first getting everything necessary organised in a yaml file and once we are sure we have everything, open a new collection in the DB for CORSIKA parameters (will require some thought about versioning).

That yaml file already exists (data/corsika/corsika_parameters.yml) and in fact also contains these spheres. I suggest modifying that yaml file to fit exactly what you need (including adding the unit and setting the name as you wish it to be) and use that. We will propagate it later to the DB and modify the way you read it.

GernotMaier commented 2 years ago

Corsika Z depends on the telescope structure definition, and I don't want to mix those two concepts. Also the outside world is providing altitude at each position, so I would rather have this quantity being the base and allow for easy re-calculation.

Ok - I will work with a yaml file for now with the definition (actually deleted it in the draft, but will add it again).

Do you have a good suggestion on how to get the telescope type/class from the name? Currently, it relies on deriving from the first letter of a name the type ('M-01'-> MST). This fails now, as we also deal with array elements which are not telescopes, but starting with the letter 'M' (e.g., MSPN-01). I think the best way forward is to change the gammasim-tools naming to MST/LST/ etc (to align with CTAO conventions)

orelgueta commented 2 years ago

I wasn't aware that we are using M-01 anywhere. The only names I am familiar with in gammasim-tools are the full names North-MST-NectarCam-D which are composed of the site, class (should be type), type (should be suffix). So I am not sure what you meant by changing the naming in gammasim-tools.

GernotMaier commented 2 years ago

I did not integrate the tool compare_layouts.py from the telescope-layouts repository and suggest no to do this at this point.

The tool compares two list of telescope positions and prints differences. This was very useful when optimising the array layouts and when layout files have been exchanged between different people. I did not use it since a very long time and I don't see an application for it now.

Ok with everyone? I would close this issue then.

orelgueta commented 2 years ago

Sure, can always reopen if we see a need for this tool

GernotMaier commented 2 years ago

Might be only in the layout files, e.g.

https://github.com/gammasim/gammasim-tools/blob/master/data/layout/telescope_positions-South-4MST.ecsv

I suggest to change the telescope names in these files to have always LST/MST/SST etc. Or do we want to move to the CTAO-official names of LSTS/LSTN ?

On 22. Sep 2022, at 10:01, orelgueta @.***> wrote:

I wasn't aware that we are using M-01 anywhere. The only names I am familiar with in gammasim-tools are the full names North-MST-NectarCam-D which are composed of the site, class (should be type), type (should be suffix). So I am not sure what you meant by changing the naming in gammasim-tools.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were assigned.


Dr Gernot Maier CTA Deutsches Elektronen-Synchrotron DESY Ein Forschungszentrum der Helmholtz-Gemeinschaft Platanenallee 6, D-15738 Zeuthen, Germany

Tel.: +49 33 7627 7598 Internet: www.desy.de/cta Besucheradresse: 1X07

orelgueta commented 2 years ago

I would like to move to the CTAO-official names. It would require quite a lot of changes and a large change to the DB, but it's better to do it as soon as possible I would say (as long as we get a promise those are the final names).

We could wait with this change until after 0.4.0 or until after the mini-release, but it's a change we should in my opinion.

GernotMaier commented 1 year ago

This was done and we agreed to close.