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

Muon reco changes #736

Merged
merged 4 commits into from
May 4, 2018
Merged

Muon reco changes #736

merged 4 commits into from
May 4, 2018

Conversation

rlopezcoto
Copy link
Contributor

  • Ring size container was previously created but not filled in the muon reco functions.

  • As discussed during the meeting, added the possibility of analyzing muon rings without applying image cleaning. Set the cleaning option to True to make not to mess with the results people were getting in the past. This option, together with all those included at the beginning of muon_reco_functions.py should eventually be moved to an input card (@AMWMitchell are you planning to do it? I could help out if you want).

@codecov
Copy link

codecov bot commented May 3, 2018

Codecov Report

Merging #736 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #736      +/-   ##
==========================================
- Coverage   69.01%   68.98%   -0.03%     
==========================================
  Files         196      196              
  Lines       10537    10541       +4     
==========================================
  Hits         7272     7272              
- Misses       3265     3269       +4
Impacted Files Coverage Δ
ctapipe/image/muon/muon_reco_functions.py 0% <0%> (ø) ⬆️

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 f810aec...a87b90f. Read the comment docs.

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.

Yes, all of the hard-coded bits need to eventually move to config variables or a table that is loaded up from ctapipe_resources. But that can be a future refactoring

@kosack
Copy link
Contributor

kosack commented May 3, 2018

Can you add a simple test case to the unit tests with this option disabled?

@rlopezcoto
Copy link
Contributor Author

Can you add a simple test case to the unit tests with this option disabled?

Unfortunately as it is implemented right now, the True value is hardcoded and it is difficult to write a test unit at the moment. I could implement this option as an input, but if all these variables are going to be moved to an input card/configuration file, it is probably not worth it to do it at the moment.
Anyways and as i said in the PR, the default value makes the code work as it was working up to now

@kosack
Copy link
Contributor

kosack commented May 4, 2018

Ok, I'll merge it now then, and we can leave refactoring the whole muon code to a later point

@kosack kosack merged commit 4fe6487 into cta-observatory:master May 4, 2018
watsonjj added a commit to watsonjj/ctapipe that referenced this pull request May 4, 2018
* master:
  Correct dl1.py for using inst.num_channels for the integration correction (cta-observatory#730)
  Muon reco changes (cta-observatory#736)
  Changes to TargetIOEventSource (cta-observatory#732)
  Improve ArrayDisplay and SubarrayDescription (cta-observatory#723)

# Conflicts:
#	ctapipe/io/containers.py
watsonjj added a commit to watsonjj/ctapipe that referenced this pull request May 4, 2018
* master:
  install protozfits v0.44.5 and require it for nectarcam (cta-observatory#734)
  Correct dl1.py for using inst.num_channels for the integration correction (cta-observatory#730)
  Muon reco changes (cta-observatory#736)
  Changes to TargetIOEventSource (cta-observatory#732)
  Improve ArrayDisplay and SubarrayDescription (cta-observatory#723)

# Conflicts:
#	ctapipe/io/containers.py
#	ctapipe/io/tests/test_nectarcameventsource.py
@rlopezcoto rlopezcoto deleted the muondev branch May 4, 2018 09:53
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.

2 participants