Skip to content
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

Additional metadata from pyhessio #655

Closed
wants to merge 5 commits into from
Closed

Additional metadata from pyhessio #655

wants to merge 5 commits into from

Conversation

morcuended
Copy link
Member

Added new parameters from pyhessio to MCHeaderContainer (as asked in image-extractor#14):

  • CORSIKA and sim_telarray version
  • energy range of primary particle
  • magnetic field
  • spectral index
  • prod site information

Check this notebook.

Note that some of the implemented fields (prod_site_array, prod_site_coord , prod_site_subarray) are still not retrieving the required information from pyhessio.

@STSpencer
Copy link
Contributor

Hi Daniel,
Do you have any idea why the three site arrays aren't being retrieved?

@morcuended
Copy link
Member Author

Hi Samuel,
No idea. I couldn't find these fields in pyhessio/hessioxxx, probably due to my limited knowledge of them. They must be somewhere in the runheader block of simtel files.
I saw you did something similar a few months ago. Do you any idea how we might get this information?

@kosack
Copy link
Contributor

kosack commented Feb 22, 2018

The tests currently fail since we don't have a package for the latest pyhessio yet, so when the test-runner installs it, the functions you use are not available. I'll try to make one shortly.

We should also add a minimum pyhessio version number in environment.yml.

@STSpencer
Copy link
Contributor

Nope, sorry. As far as I recall, it was the case that it wasn't implemented in pyhessio whatsoever, which seems very odd. It's a shame as it'd be an extremely useful feature for machine learning studies with multiple telescopes.

@GernotMaier
Copy link
Contributor

Why not adding the possibility the retrieve the full run header?

e.g. viewcone radius, details on interaction models, wavelength ranges, bunch sizes, how often each shower is used, etc?

Here an example of what was useful in the past:

MC run header:
Shower program: 1 (CORSIKA) version 6990
Detector program: 1 (sim_telarray) version 1462392225 (2016-05-04 22:03:45 CEST)
Shower simulation started 2016-07-11, detector simulation started 2016-07-11
Observation height: 2150.000000 m
Number of showers to be simulated: 20000
Number of arrays per shower: 20
Core position mode: 1 (circular)
Core range: 0.000000, 3000.000000 m
Altitude range: 70.000000 to 70.000000 deg
Azimuth range: 180.000005 to 180.000005 deg
Diffuse mode: 1
Checking units of viewcone parameters: in degrees!
Viewcone: 0.000000 to 10.000000 deg
Energy range: 0.003 to 330 TeV
Spectral index: -2.000000
B field total: 23.117723 microT, incl.: -22.712710 deg, decl.: 0.000000 deg
Injection height: -0.001000 km
Atmospheric density profile: 26
IACT options: 187
Low energy model: 2 (URQMD)
High energy model: 2 (QGSJET)
Maximum bunchsize: 5.000000
Wavelength range: 240.000000 to 700.000000 nm
Details on high-E model: version flag = 3, cross section flag = 3
This is QGSJET-II.

@morcuended
Copy link
Member Author

That would be the best, Gernot. I will work on it.

@bryankim96
Copy link
Member

Are there any updates on adding these header fields?

@morcuended
Copy link
Member Author

Unfortunately, there is nothing new on my side.

@thomasgas
Copy link

Hi @morcuended!
I was about to make a PR on this in order to add the altitude of the observation site to the MCHeaderContainer because by know it is hard-coded:

but I saw that you already started to implement some features.
Since I would like to add these and other useful informations from the simtel file using pyhessio, would you mind if I continue to implement some features?
@kosack in #466 you said the MCHeaderContainer had to be removed: should we complete this task (having a real MCHeaderContainer with the informations listed by @GernotMaier) or not?
ideas?
Related to: #500 and #722 from @watsonjj

@codecov
Copy link

codecov bot commented Oct 13, 2018

Codecov Report

Merging #655 into master will increase coverage by 4.89%.
The diff coverage is 86.97%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #655      +/-   ##
==========================================
+ Coverage   66.98%   71.88%   +4.89%     
==========================================
  Files         190      204      +14     
  Lines       10129    11014     +885     
==========================================
+ Hits         6785     7917    +1132     
+ Misses       3344     3097     -247
Impacted Files Coverage Δ
ctapipe/image/muon/tests/test_muon_fitting.py 100% <ø> (ø)
ctapipe/tools/plot_charge_resolution.py 27.11% <ø> (ø) ⬆️
ctapipe/image/tests/test_pixel_likelihood.py 100% <ø> (ø) ⬆️
ctapipe/analysis/camera/chargeresolution.py 85.04% <ø> (ø) ⬆️
ctapipe/coordinates/__init__.py 100% <ø> (ø) ⬆️
ctapipe/utils/fitshistogram.py 79.71% <ø> (ø) ⬆️
ctapipe/image/muon/tests/test_muon_features.py 100% <ø> (ø)
...ipe/tools/plot_charge_resolution_variation_hist.py 30% <ø> (ø) ⬆️
ctapipe/instrument/optics.py 94.23% <ø> (ø) ⬆️
ctapipe/image/charge_extractors.py 89.69% <ø> (ø) ⬆️
... and 120 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 e976f00...dc5c12a. Read the comment docs.

@morcuended
Copy link
Member Author

Sure Thomas, go ahead.

@thomasgas
Copy link

thomasgas commented Oct 15, 2018

Sure Thomas, go ahead.

ok so I'll wait for a reply from @kosack and @watsonjj in order to understand if it is worth the effort or if there is a refactoring of the containers in mind which would make such contribution useless.

@kbruegge
Copy link
Member

We want to put things like observation height into the instrument/subarray configuration.

@kbruegge
Copy link
Member

Are these fields covered in the pyeventio? @maxnoe

@maxnoe
Copy link
Member

maxnoe commented Dec 12, 2018

Yes, you could access them from the mc_runheaders variable of the SimTelFile

@kbruegge
Copy link
Member

Great. So we can close this. 👍

watsonjj added a commit to watsonjj/ctapipe that referenced this pull request Jan 7, 2019
* master: (22 commits)
  Remove all hillas_paramters but version 5, fixes cta-observatory#866  (cta-observatory#904)
  Fix docstring of EventSeeker, fixes cta-observatory#768 (cta-observatory#914)
  Do not set autodownload, fixes doc build (cta-observatory#910)
  Import bokeh.palletes correctly, fixes cta-observatory#911 (cta-observatory#912)
  Fix SST-1M to be DC and not SC. Solves cta-observatory#905 (cta-observatory#908)
  Fix a few test warnings (cta-observatory#902)
  Updates of NectarCam and LSTCam real data eventsource class (cta-observatory#812)
  Implemented FACT image cleaning (cta-observatory#862)
  remove `config=None, tool=None` in some tests (cta-observatory#897)
  Remove flow submodule (got moved to its own package) (cta-observatory#893)
  Cleaning the ctapipe folder. this has been empty for 3 years. (cta-observatory#892)
  Additional metadata from pyhessio, similar to cta-observatory#655 (cta-observatory#895)
  add scikit-learn to install_requires (cta-observatory#877)
  Add .mailmap (cta-observatory#894)
  Fix subarray peek. No more warnings. (cta-observatory#841)
  SimTelEventSource using pyeventio (cta-observatory#864)
  Use sparse neighbor matrix (cta-observatory#826)
  Add test how it should be (cta-observatory#842)
  fix errordef > 0. (cta-observatory#839)
  Fix warning about already closed hessio file (cta-observatory#834)
  ...

# Conflicts:
#	ctapipe/analysis/camera/chargeresolution.py
#	ctapipe/tools/extract_charge_resolution.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants