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

Removed the duplicated masks for DL1 parameters & images #478

Merged
merged 5 commits into from
Jul 17, 2020
Merged

Removed the duplicated masks for DL1 parameters & images #478

merged 5 commits into from
Jul 17, 2020

Conversation

moralejo
Copy link
Collaborator

Removed the duplicated masks for the DL1 parameters and DL1 images tables,
since they have one-to-one corresponding rows. Besides, code did not work
because images table now lacks event_id

…bles,

since they have one-to-one corresponding rows. Besides, code did not work
because images table now lacks event_id
@codecov
Copy link

codecov bot commented Jul 12, 2020

Codecov Report

Merging #478 into lstchain_ctapipe0.8 will increase coverage by 0.03%.
The diff coverage is 0.00%.

Impacted file tree graph

@@                   Coverage Diff                   @@
##           lstchain_ctapipe0.8     #478      +/-   ##
=======================================================
+ Coverage                42.03%   42.06%   +0.03%     
=======================================================
  Files                       76       76              
  Lines                     6290     6285       -5     
=======================================================
  Hits                      2644     2644              
+ Misses                    3646     3641       -5     
Impacted Files Coverage Δ
lstchain/datachecks/containers.py 41.81% <0.00%> (ø)
lstchain/datachecks/dl1_checker.py 6.79% <0.00%> (+0.04%) ⬆️

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 4c9118e...c2ae722. Read the comment docs.

@maxnoe
Copy link
Member

maxnoe commented Jul 12, 2020

images table now lacks event_id

That should not be, everything needs to be identifiable

calculation of telescope efficiency with muon rings.
@moralejo
Copy link
Collaborator Author

images table now lacks event_id

That should not be, everything needs to be identifiable

I thought it was by design, since the rows in the two tables correspond one to one, so the event_id in both would be redundant. As long as things work properly, of course! If we want absolute safety we could put back that event_id in ctapipe...

@maxnoe
Copy link
Member

maxnoe commented Jul 13, 2020

If we want absolute safety we could put back that event_id in ctapipe...

This is clearly a bug, since the tool has the option to only write the image table

But wait: you are talking about the lstchain dl1 processing, not the ctapipe tool? Then this has nothing to do with ctapipe, it needs to be fixed here!

@moralejo
Copy link
Collaborator Author

But wait: you are talking about the lstchain dl1 processing, not the ctapipe tool? Then this has nothing to do with ctapipe, it needs to be fixed here!

This happened as a result of a change in ctapipe, but indeed it makes no sense to change anything in ctapipe (or even in lstchain) since we will move as soon as possible to the new DL1. So no need to do anything about this.

@moralejo moralejo marked this pull request as draft July 13, 2020 10:11
@maxnoe
Copy link
Member

maxnoe commented Jul 13, 2020

This happened as a result of a change in ctapipe,

The containers don't duplicate this information anymore, this is right. For writing it out, we use the EventIndex and TelescopeEventIndex containers.

@moralejo moralejo marked this pull request as ready for review July 13, 2020 10:26
@moralejo moralejo merged commit 73551a9 into cta-observatory:lstchain_ctapipe0.8 Jul 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants