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

Fix DL1 EventSource #1546

Merged
merged 24 commits into from
Dec 3, 2020
Merged

Conversation

LukasNickel
Copy link
Member

Small changes to tools and bugfixes in order to work with the DL1EventSource

  • Fix indexing bug in DL1EventSource  #1545 adding a new method to SubarrayDescription
  • Fix event generation in the DL1EventSource: Every second event was skipped, because the iterator was advanced twice. That's a huge bug if it didn't work for some random other reason before
  • Rename dl1eventsource.mc_headers to dl1eventsource.simulation_config in line with the changes to the SimtelEventSource and container structure changes. To make this behave the same way the SimtelEventSource does when using e.g. the stage-1 tool, this returns a single container if the file contains only one simulation run. This makes it somewhat inconsistent for merged files, but I think there a a lot of things to clean up anyway for us to fully support merged dl1 files consistently.
  • Add tests using the stage1 and muon reconstruction tools on dl1 files

- Dl1 files contain a boolean mask, we want a list of tel_ids
- This is now done with the `tel_mask_to_tel_ids` method
- Only calibrate events if no images are present
- Crash if file contains no images or raw data
- Iterate dl1 telescopes, not raw event. This is needed if the file
contains no raw data.
- In line with the global replacement of `mc` to `simulation`
- Enables one to calculate DL1B data from DL1A files.
The saved cleaning mask is applied, specifying a cleaning has no effect
right now
- Muon reconstruction and stage1 work with dl1a data now
- simulation_configs changed to simulation_config for now to match the
SimtelEventSource
ctapipe/tools/tests/test_tools.py Outdated Show resolved Hide resolved
ctapipe/tools/tests/test_tools.py Outdated Show resolved Hide resolved
- Use the HDF5TableReader to retrieve the obs_id as well
- Setting the scope to module avoids recalculation of the same files
- Using tmpdir, its not necessary to use with() statements to create
temporary files
@maxnoe maxnoe changed the title Dl1 follow up Fix DL1 EventSource Nov 23, 2020
@maxnoe maxnoe added this to the v0.10.1 milestone Nov 23, 2020
- This only worked by accident before
- Images default to None in the container, so this checks if
information was read
@codecov
Copy link

codecov bot commented Nov 24, 2020

Codecov Report

Merging #1546 (2399d0f) into master (3c27fc3) will increase coverage by 0.16%.
The diff coverage is 97.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1546      +/-   ##
==========================================
+ Coverage   90.62%   90.78%   +0.16%     
==========================================
  Files         192      192              
  Lines       13840    13897      +57     
==========================================
+ Hits        12542    12616      +74     
+ Misses       1298     1281      -17     
Impacted Files Coverage Δ
ctapipe/io/dl1writer.py 83.64% <ø> (ø)
ctapipe/io/dl1eventsource.py 89.38% <88.00%> (+8.24%) ⬆️
ctapipe/instrument/subarray.py 94.79% <100.00%> (+0.05%) ⬆️
ctapipe/io/eventsource.py 93.75% <100.00%> (+0.13%) ⬆️
ctapipe/io/tests/test_dl1eventsource.py 100.00% <100.00%> (+4.16%) ⬆️
ctapipe/tools/display_dl1.py 90.47% <100.00%> (+0.47%) ⬆️
ctapipe/tools/muon_reconstruction.py 90.90% <100.00%> (+0.07%) ⬆️
ctapipe/tools/stage1.py 100.00% <100.00%> (ø)
ctapipe/tools/tests/test_tools.py 98.44% <100.00%> (+0.31%) ⬆️
ctapipe/tools/dl1_merge.py 85.02% <0.00%> (-1.07%) ⬇️
... and 3 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 3c27fc3...2399d0f. Read the comment docs.

@kosack
Copy link
Contributor

kosack commented Nov 25, 2020

This is marked for 0.10.1, but also says "API change". I see only that the mc_header attribute changed to simulation_config (as it should, this was missed in the previous release). I think that is less of an API change than a API bugfix, since the API for EventSources already had that attribute. Are there other larger API changes here? Or can we remove that tag, so it can go in 0.10.1 vs 0.11.0?

@LukasNickel
Copy link
Member Author

LukasNickel commented Nov 25, 2020

This is marked for 0.10.1, but also says "API change". I see only that the mc_header attribute changed to simulation_config (as it should, this was missed in the previous release). I think that is less of an API change than a API bugfix, since the API for EventSources already had that attribute. Are there other larger API changes here? Or can we remove that tag, so it can go in 0.10.1 vs 0.11.0?

I think thats all, so API bugfix might be more accurate.
But there is no tag for that :)

ctapipe/tools/stage1.py Outdated Show resolved Hide resolved
ctapipe/tools/stage1.py Outdated Show resolved Hide resolved
- from_config is not needed, if the eventsource uses the baseclass
constructor properly
Copy link
Contributor

@kosack kosack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

One minor comment: the self.datalevels property is accessed several times in the event loop, so it may be better either as a lazyproperty, or better yet to generate the values in the constructor and just return self._datalevels in the property. The latter may be preferred as right now if you call close(), accessing the datalevels property causes an exception since _h5file is no longer open.

Otherwise, once the conflicts are fixed, this looks ready.

- Datalevels are checked multiple times when generating events,
this avoids having to call this code every time
- Datalevels are remmebered even if the file is closed
@kosack kosack merged commit 78c98a2 into cta-observatory:master Dec 3, 2020
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.

indexing bug in DL1EventSource
3 participants