Refactor instrument module

[maxnoe]

Working on #971

• Only guesses telescope names, not full camera geometries or optics
• Refactoring of the members of TelescopeDescription, OpticsDescription
• Make those classes hashable, so set operations work and they can be used as dict keys

[dneise]

Btw, I was curious if one can find the telescope names somewhere in the eventio file, so that guessing (even of the name) could potentially avoided completely. It found them in the "History" part, I think, but only in comments looking like this:

ECHO ************************************************************************
ECHO Configuration for single-reflector SST-1M 5 in Prod-4.
ECHO ************************************************************************
ECHO 
ALTITUDE 2000. % m (Typically replaced on the command line).
ATMOSPHERIC_TRANSMISSION atm_trans_2000_1_10_0_0_2000.dat
TRIGGER_TELESCOPES 1           % Unless an array trigger file sets up more specific triggers.
[.. more configuration ..]

I guess this is not a good way to read the telescope name from the file, but I wanted to mention it anyway.

[maxnoe]

The tests now failing are due to testing for magic numbers in the old simtel file.

Any advice on how to replace those values?

[kbruegge]

The tests now failing are due to testing for magic numbers in the old simtel file.

Any advice on how to replace those values?

If we find no other way to check for correct values then these tests can be removed imo.

[kosack]

I guess this is not a good way to read the telescope name from the file, but I wanted to mention it anyway.

I know that I had mentioned that it would be nice to have the names to Konrad, and I think he said he would add that in a future version. It may be there already somewhere. In any case, we still probably want to map names there to "standard" ones we define, so there is no confusion.

[kosack]

If we find no other way to check for correct values then these tests can be removed imo.

was the example file updated? If so, yes we should remove all tests that assume specific values. It's better to give the tests a hand-made dummy event where we know the correct outcome, rather than using a real event (which can be tested at a higher level if necessary). Probably that should be a separate PR.

[kosack]

Maybe not necessary for this PR, but it would be nice to add a "guess" function for the SubarrayDescription itself, so that at least the Prod ID and the array layout could be shown (not sure if that is info directly in the MC headers or not). Right now all subarrays are called "MonteCarloArray", which is not so useful to identify them.

[maxnoe]

I know that I had mentioned that it would be nice to have the names to Konrad, and I think he said he would add that in a future version. It may be there already somewhere. In any case, we still probably want to map names there to "standard" ones we define, so there is no confusion.

I just produced small test files using the current developement version of simtel_array (prod4 baselines la palma and paranal) and I found no changes in the containers that describe the telescopes.

[maxnoe]

was the example file updated? If so, yes we should remove all tests that assume specific values. It's better to give the tests a hand-made dummy event where we know the correct outcome, rather than using a real event (which can be tested at a higher level if necessary). Probably that should be a separate PR.

Yes, I changed the example_event to come from the prod3 gamma_test_large.simtel.gz

[maxnoe]

I removed the magic number tests, test pass now.
I opened an issue here: #988 to get real tests for those features.

[codecov]

Codecov Report

Merging #982 into master will increase coverage by 0.52%.
The diff coverage is 93.29%.

@@            Coverage Diff             @@
##           master     #982      +/-   ##
==========================================
+ Coverage   78.19%   78.71%   +0.52%     
==========================================
  Files         194      197       +3     
  Lines       11313    11334      +21     
==========================================
+ Hits         8846     8922      +76     
+ Misses       2467     2412      -55

Impacted Files	Coverage Δ
ctapipe/image/muon/muon_reco_functions.py	66.66% <ø> (ø)	⬆️
ctapipe/tools/display_dl1.py	90.09% <ø> (ø)	⬆️
ctapipe/io/tests/test_targetio_event_source.py	6.5% <0%> (ø)	⬆️
ctapipe/io/tests/test_hessio_event_source.py	100% <100%> (ø)	⬆️
ctapipe/calib/camera/tests/test_dl1.py	100% <100%> (ø)	⬆️
ctapipe/plotting/tests/test_camera.py	100% <100%> (ø)	⬆️
ctapipe/instrument/guess.py	100% <100%> (ø)
ctapipe/io/tests/test_prod2.py	100% <100%> (ø)
ctapipe/instrument/__init__.py	100% <100%> (ø)	⬆️
ctapipe/instrument/tests/test_optics.py	100% <100%> (ø)	⬆️
... and 40 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dcb64d4...7f2c059. Read the comment docs.

[maxnoe]

Tests are passing, please review @watsonjj @kosack @dneise @ParsonsRD @mackaiver

[kbruegge]

So the difference between the two test fles is the MC production? Prod2 vs Prod3? Maybe we should rename that test file to something conveying more information.

[maxnoe]

Yes, but this would be fore ctapipe-extra. I can also produce more sensible test files not, I got a recent simtel running.

[maxnoe]

Please review again, I added @watsonjj comment.

[kosack]

Yes, but this would be fore ctapipe-extra. I can also produce more sensible test files not, I got a recent simtel running.

It would be nice to have some smaller nicer test files and get rid of the old ones in ctapipe_extra which is becoming too large already: at least something like 10+ gamma rays for north and south configurations, and some muon-only files. For deeper/larger test files, we can just make them automatically download from the server, rather than put them in the package. In the future, we will move to selectable configurations as in #738, so this is just a temporary fix.