[RF] Added thorough parameter study for RooCBShape and RooJohnson #16893
+132
−29
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Patrik1van
requested review from
lmoneta,
guitargeek and
couet
as code owners
guitargeek
reviewed
Jan 6, 2025
| ###References: | ||
| Johnson, N. L. (1949). *Systems of Frequency Curves Generated by Methods of Translation*. Biometrika **36(1/2)**, 149–176. [doi:10.2307/2332539](https://doi.org/10.2307%2F2332539) | ||
|
|
||
| \image html RooJohnson_plot.png |
There was a problem hiding this comment.
Why not use this plot anymore? I liked it, because it nicely illustrated the shape of the RooJohnson for different values of the parameter in one canvas:
https://root.cern/doc/v632/classRooJohnson.html
I think even with your added parameter studies, it's still a nice complementary plot.
guitargeek
requested changes
Jan 6, 2025
There was a problem hiding this comment.
Thank you very much @Patrik1van for the nice improvements to the docs!
If you still have time to work on this, I have a few change requests:
- Could you please provide the code that is used to generate these plots? Maybe put it in a code block of the PR description. It would be good if we can easily reproduce these plots in case they need a different formatting, colors, etc.
- Can you give the png files less generic names? Right now,
combined_sigmas.pngfor example doesn't tell me at all that this is related to the RooCBShape. Maybe call itRooCBShape_sigmas.png? Same pattern for the other files. - The existing RooJohnson documentation uses thicker line widths, which I think is very good. Can you do the same?
And when you create an updated branch with these changes, can you please force-push it to the branch with the same name documentation_changes_cb_john so that you don't have to open a new PR? Thank you very much!
guitargeek
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.