-
Notifications
You must be signed in to change notification settings - Fork 77
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
Tool to produce a time calibration HDF5 file #533
Conversation
Codecov Report
@@ Coverage Diff @@
## master #533 +/- ##
==========================================
- Coverage 37.46% 37.23% -0.24%
==========================================
Files 83 84 +1
Lines 7425 7471 +46
==========================================
Hits 2782 2782
- Misses 4643 4689 +46
Continue to review full report at Codecov.
|
help="Path to the input file containing events (wildcards allowed)", | ||
).tag(config=True) | ||
|
||
output_file = traits.Unicode( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Paths should be ctapipe.core.traits.Path
traitlets
reader = LSTEventSource(input_url=self.path_list[0], max_events=self.max_events) | ||
self.lst_r0 = LSTR0Corrections( | ||
pedestal_path=self.pedestal_file, | ||
config=self.config, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All sub-components of a tool need to be passed parent=self
instead of config=self.config
to work properly
|
||
try: | ||
for j, path in enumerate(self.path_list): | ||
reader = LSTEventSource(input_url=self.path_list[0], max_events=self.max_events) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the eventsource get path
as input? Like this you are only looking at the first file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All path-like arguments should be changed from traits.Unicode
to ctapipe.core.traits.Path
.
All sub-components of this tool need to get the tool as parent, not the config.
So everywhere, exchange config=self.config
with parent=self
.
This is required for the config system to work!
To be able to store the used config in the proveance system, also self.add_component
must be called.
So all subcomponents should be created like this:
class MyTool(Tool):
...
def setup(self):
self.subcomponent = self.add_component(SubComponent(parent=self))
Have a look at the ctapipe stage 1 tool here:
I have updated this PR with requested changes (when possible) addressed and also new additions:
|
If these are already traitlets of the corresponding classes, there is no need to repeat them. |
You're right. |
Continued in #600 |
This PR adds a new Tool
lstchain_create_time_calibration_file