-
Notifications
You must be signed in to change notification settings - Fork 272
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor/improve definition and use of TelescopeDescription and its string represenation #2675
Comments
This came up multiple times, but we don't seem to have an issue for it, at least a quick search doesn't bring on up. To comment, we have multiple conflicting wanted behaviors with respect to telescope descriptions and their string versions. Some observations
This leads to e.g. same string but different types in Prod6 for the two LSTs. Originally, this affected only inofficial LST + MAGIC simulations, and we change the assumption that same string means same data to support these simulations, see #1424 and 1426. But now this is even the case in official Prod6 simulations. In the case of the LSTs, this is due to differences in the camera (readout) definition:
particularly the single pe pulse shape. We use the string mostly to say "This is sufficiently similar to treat it as the same telescope type in this context". But I think we just need to let go of this concept and be explicit when we want to group together which telescopes and how. E.g. we use it to train models or load events for the same "type". |
It's also when viewing a plot of the array, it's nice to have a human-readable version that shows what was done, but it's true that that doesn't need to exactly match the training division. In any case, as long as we provide the metadata we want when generating the simulations, that is solvable. So the question is more for grouping for training, if we need a better interface or not. Either it could be very explicit, like a list of telescopes, but that is a bit annoying to have to specify in the configuration. We could also just make a nicer grouping function that returns the actually different groups by comparing the OpticsDescription and CameraDescriptions correctly, and just do something like append an index if two have the same string rep (e.g. it would return a set of groups and names like: |
This also makes me wonder why we have the same string rep for different types in the first place, but good to fix. Really, the telescope type string was supposed to have three parts:
When anything changes in the description, the optics or camera name should also be changed, i,e by adding a version, but we don't enforce that. The optics name should not in general be just "LST" (which is redundant), but rather something version-able or that describes better the current optics design. I would expect in the future we could have e.g.
1: LST_v1_LSTCam
,2,3,4: LST_v2_LSTCam
, but that needs to be done at the simualtion production level when setting the metadata.Originally posted by @kosack in #2673 (review)
The text was updated successfully, but these errors were encountered: