cta-observatory / ctapipe

Low-level data processing pipeline software for CTAO or similar arrays of Imaging Atmospheric Cherenkov Telescopes
https://ctapipe.readthedocs.org
BSD 3-Clause "New" or "Revised" License
63 stars 267 forks source link

Add docstring for ctapipe-train-disp-reconstructor #2420

Closed LukasBeiske closed 10 months ago

LukasBeiske commented 10 months ago

Fixes #2283

ccossou commented 10 months ago

I'm trying to review this, @LukasBeiske are you available now? I have a few questions.

maxnoe commented 10 months ago

It's also missing from the sphinx documentation, could you add it please?

maxnoe commented 10 months ago

https://github.com/cta-observatory/ctapipe/blob/main/docs/api-reference/tools/index.rst

ccossou commented 10 months ago

As far as I understand, this solves the issue, but I don't understand why the corresponding docstring has to be at the top of the file, instead of just using the one that already exist in the tool class TrainDispReconstructor.

maxnoe commented 10 months ago

@ccossou I agree, the ctapipe-info tool should use the class docstring, not the module docstring

ccossou commented 10 months ago

Am I supposed to approve it now, even though we still need to change the documentation (cf comment by max)?

I assume our comments about using the class docstring are more for a future pull request about a refactoring of ctapipe-info and shouldn't block this one, but that's my opinion only.

maxnoe commented 10 months ago

I assume our comments about using the class docstring are more for a future pull request about a refactoring of ctapipe-info and shouldn't block this one, but that's my opinion only.

Yes, let's do this separately.

maxnoe commented 10 months ago

You need to replace some single ticks with double ticks in the docstrings:

https://github.com/cta-observatory/ctapipe/actions/runs/6669605856/job/18127742839?pr=2420#step:6:124

LukasBeiske commented 10 months ago

I noticed that the default target column of the DispReconstructor is still called true_norm. This target column never gets written out anywhere, since it gets calculated just before training the models, so its name does not really matter, but the target column that gets calculated is actually true_norm * true_sign. To avoid unnecessary confusion, I would suggest just naming this target column true_disp. If you agree, should I just add that change here, since it changes only a single line of code?