-
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 generate a DL2 file from a DL1 file #599
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #599 +/- ##
========================================
Coverage 83.16% 83.16%
========================================
Files 54 56 +2
Lines 4353 4455 +102
========================================
+ Hits 3620 3705 +85
- Misses 733 750 +17 ☔ View full report in Codecov by Sentry. |
0187642
to
96b1b7b
Compare
|
||
def setup(self): | ||
|
||
self.log.info("Reading configuration") |
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.
Tool already supports reading configuration files. Please don't add any new configuration file logic. Adapt the tool / config to work with the existing configuration file logic.
There should absolutely be no code to deal with configurations in here.
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.
We loose most of the advantages of using Tools if we again are not using it how intendend but reimplement a new configuration system on top of it.
self.log.info("Reading DL1 file") | ||
self.data = pd.read_hdf(self.input, key=dl1_params_lstcam_key) | ||
self.data = add_delta_t_key(self.data) | ||
if self.configuration["source_dependent"]: |
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.
E.g. this should be solved by adding a traitlet source_dependent = Bool().tag(config=True)
. and then just checking if self.source_dependent
here.
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.
Please do not reimplement the configuration loading system.
Since many of the options are for functions / things further down, the best way would do convert those into Components
as well. But that could be left for another PR. Having the tool is a good start, but it should use traitlets / the config system correctly, otherwise we're not much better off than with the scripts.
("i", "input"): "ReconstructionHDF5Writer.input", | ||
("o", "output"): "ReconstructionHDF5Writer.output_dir", | ||
"models": "ReconstructionHDF5Writer.path_models", | ||
"config": "ReconstructionHDF5Writer.config_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.
No need for this, Tool
already comes with config
traitlet for setting the configuration file
output_dir = traits.Path( | ||
help="Path where to store the reconstructed DL2", file_ok=False | ||
).tag(config=True) | ||
config_file = traits.Path( |
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.
This is not needed, as Tool
already implements the configuration handling.
self.data = pd.concat([self.data, data_src_dep], axis=1) | ||
|
||
self.log.info("Reading RF models") | ||
self.reg_energy = joblib.load(self.path_models / "reg_energy.sav") |
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.
In general, I am not a big fan of setting an input directory and then have hard coded names for files in that input directory. I would go for three Path
traitlets for each of the models. This is much more flexible for e.g. debugging / benchmarking different models / what not.
self.cls_gh = None | ||
self.dl2 = None | ||
|
||
def setup(self): |
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.
I think setup
should only initialize components that need the configuration. Any actual work, like loading input data and so on should happen in run
.
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.
do you suggest moving everything that is now in setup
to start
?
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.
No, the code checking the traitlets and so on belongs here, but I would move the loading of the data and the models.
Any actual work should be done in start
rather than setup
self.data.az_tel = -np.pi / 2.0 | ||
self.data = filter_events( | ||
self.data, | ||
filters=self.configuration["events_filters"], |
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.
Add a List
traitlet for this and give filters=self.filters
self.data = filter_events( | ||
self.data, | ||
filters=self.configuration["events_filters"], | ||
finite_params=self.configuration["regression_features"] |
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.
Same here, add a List
traitlet and add self.regression_features
.
self.data, | ||
filters=self.configuration["events_filters"], | ||
finite_params=self.configuration["regression_features"] | ||
+ self.configuration["classification_features"], |
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.
And again. Do not use self.configuration
!
4f2caf3
to
d1d009e
Compare
Thanks @maxnoe for the code review. I have tried to set the config file parameter as mandatory, and worked with and internal Otherwise, tests with data have been added and we have green light in the CI. |
This is exactly what I wanted to avoid! Still using a separate configuration system.
Traits define what can be in a config file! That's the whole point of them. That you can also set these via the command line is a bonus. |
self.reg_disp_vector = None | ||
self.cls_gh = None | ||
|
||
self._dict_conf = replace_config(standard_config, self.get_current_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.
Still using a separete config system. Please remove and use traitlets properly. Otherwise the switch to Tools is kind of pointless.
self.cls_gh, | ||
self.reg_energy, | ||
self.reg_disp_vector, | ||
custom_config=self._dict_conf, |
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.
If you want to be minimally invasive, construct custom_config
from the traitlets here, but the better approach would be to implement a Component
for apply_models
.
5cd77c8
to
994a17a
Compare
Closing as this is stalled. |
This PR is the continuation of #573 developed in a forked repo and now with the code placed in a branch of
lstchain
repo.It adds a new Tool
lstchain_create_dl2_file
adapting the existing code found in scriptscripts/lstchain_dl1_to_dl2.py