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

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

Merged
merged 4 commits into from Oct 31, 2023

Conversation

LukasBeiske
Copy link
Contributor

Fixes #2283

@Tobychev Tobychev added the documentation-only Label that will ensure code tests are skipped label Oct 24, 2023
Tobychev
Tobychev previously approved these changes Oct 26, 2023
@Tobychev Tobychev requested a review from ccossou October 26, 2023 14:41
@ccossou
Copy link
Contributor

ccossou commented Oct 27, 2023

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

@maxnoe
Copy link
Member

maxnoe commented Oct 27, 2023

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

@maxnoe
Copy link
Member

maxnoe commented Oct 27, 2023

@ccossou
Copy link
Contributor

ccossou commented Oct 27, 2023

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
Copy link
Member

maxnoe commented Oct 27, 2023

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

@ccossou
Copy link
Contributor

ccossou commented Oct 27, 2023

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
Copy link
Member

maxnoe commented Oct 27, 2023

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
Copy link
Member

maxnoe commented Oct 27, 2023

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
Copy link
Contributor Author

LukasBeiske commented Oct 27, 2023

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?

@maxnoe maxnoe merged commit 42e35c0 into cta-observatory:main Oct 31, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation-only Label that will ensure code tests are skipped
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ctapipe-train-disp-reconstructor missing docstring
4 participants