Make ML config files more readable and add comments. #2455
Conversation
| n_estimators: 10 | ||
| max_depth: 10 | ||
| n_jobs: -1 | ||
|
|
||
| log_target: True | ||
| log_target: True # If true, norm(disp) models predict log(norm(disp)) (output = exp(prediciton)) |
There was a problem hiding this comment.
I'm not sure I understand what the last statement in parenthesis means: it might be trying to say that the default of scikit-learn is that the models output gets wrapped by exp() before being passed to the user, though I'm not sure that is true...
There was a problem hiding this comment.
Ah no, this is not related to scikit-learn, we do this ourselves, if log_target = True. I just want to say that the model outputs the correct values, even if log_target = True.
I'm not sure if this is obvious, but writing a short sentence is probably better than this parenthesis.
There was a problem hiding this comment.
I don't think it's a good idea to document the options in the config files, they are available in the documentation:
https://ctapipe.readthedocs.io/en/latest/api/ctapipe.reco.DispReconstructor.html#ctapipe.reco.DispReconstructor.log_target
Duplicating this by hand has the danger of getting out of sync.
There was a problem hiding this comment.
@LukasBeiske I think Max has a good point that we shouldn't duplicate the existing documentation in the example config files.
Instead I suggest you open a PR where you update the DispReconstructor with the doc strings you suggest here, as I find them more useful.
There was a problem hiding this comment.
Ok, I can see that. This extends to all the config options of the ML Reconstructor classes and the stereo combiner options, as well, doesn't it?
I noticed that there are some docstrings missing in DispReconstructor and the other ML related classes, so I will put all of that in a separate PR 👍
There was a problem hiding this comment.
In that case, should I also remove these options for which I am only setting the default values here (like e.g. prefix, and StereoMeanCombiner.weights)?
There was a problem hiding this comment.
In that case, should I also remove these options for which I am only setting the default values here (like e.g.
prefix, andStereoMeanCombiner.weights)?
I think for the example config it makes more sense to change disp to something custom (maybe "elite" ; ) as an example to the reader. The stereo combiner weights is something you choose for more principled reasons so maybe remove it.
There was a problem hiding this comment.
I don't think we should change the prefixes by default. For most purposes, we should stick to (and properly define them) the default prefixes.
Otherwise, we'll end up with different prefixes / data formats for the same things done by different users. (or all of them using the same one we now defined as "new default" in the config file.
I'd say comment out the "prefix" completely and add a comment like "add a prefix here if you e.g. want to compare the same method applied with different settings".
There was a problem hiding this comment.
I think for the example config it makes more sense to change
dispto something custom (maybe "elite" ; ) as an example to the reader. The stereo combiner weights is something you choose for more principled reasons so maybe remove it.
I removed the default value for the weights and commented it out, just like prefix. I think leaving it in as a comment like this makes it a bit more obvious how one can set the options of StereoMeanCombiner or any other future stereo combiner.
LukasBeiske
force-pushed
the
verbose_ml_configs
branch
from
51a4ccd
to
933c22e
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2455 +/- ##
=======================================
Coverage 60.60% 60.60%
=======================================
Files 3 3
Lines 33 33
=======================================
Hits 20 20
Misses 13 13 ☔ View full report in Codecov by Sentry. |
I also added some more config options with their default values to make users aware of their existence.