-
Notifications
You must be signed in to change notification settings - Fork 1
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
Set best_fit_args to confidence_interval_args if None #76
Conversation
alea/model.py
Outdated
also the best-fit parameters of profile-likelihood, | ||
parameter_interval_bounds, and confidence interval | ||
confidence_interval_args (dict, optional (default=None)): Parameters that will be fixed | ||
in the profile likelihood computation. If None, all fittable parameters will be profiled except the poi |
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.
[pep8] reported by reviewdog 🐶
E501 line too long (116 > 100 characters)
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.
Usually, we require 4 spaces indent, if using VSCode, the 'indent-rainbow' extension will help you to see the indents.
I would change this into:
confidence_interval_args (dict, optional (default=None)):
Parameters that will be fixed in the profile likelihood computation.
If None, all fittable parameters will be profiled except the poi.
Similar things apply to other docstrings complained about by reviewdog.
parameter_interval_bounds, and confidence interval | ||
confidence_interval_args (dict, optional (default=None)): Parameters that will be fixed | ||
in the profile likelihood computation. If None, all fittable parameters will be profiled except the poi | ||
best_fit_args (dict, optional (default=None)): If you require the "global" best-fit used to normalise the |
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.
[pep8] reported by reviewdog 🐶
E501 line too long (117 > 100 characters)
alea/model.py
Outdated
confidence_interval_args (dict, optional (default=None)): Parameters that will be fixed | ||
in the profile likelihood computation. If None, all fittable parameters will be profiled except the poi | ||
best_fit_args (dict, optional (default=None)): If you require the "global" best-fit used to normalise the | ||
profile likelihood ratio to fix fewer parameters than the profile likelihood-- mainly used for 1-D slices |
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.
[pep8] reported by reviewdog 🐶
E501 line too long (118 > 100 characters)
alea/model.py
Outdated
in the profile likelihood computation. If None, all fittable parameters will be profiled except the poi | ||
best_fit_args (dict, optional (default=None)): If you require the "global" best-fit used to normalise the | ||
profile likelihood ratio to fix fewer parameters than the profile likelihood-- mainly used for 1-D slices | ||
of higher-dimensional confidence volumes, where the global best-fit may not be along the profile. |
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.
[pep8] reported by reviewdog 🐶
E501 line too long (110 > 100 characters)
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.
Thanks @kdund . Looks good!
alea/model.py
Outdated
also the best-fit parameters of profile-likelihood, | ||
parameter_interval_bounds, and confidence interval | ||
confidence_interval_args (dict, optional (default=None)): Parameters that will be fixed | ||
in the profile likelihood computation. If None, all fittable parameters will be profiled except the poi |
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.
Usually, we require 4 spaces indent, if using VSCode, the 'indent-rainbow' extension will help you to see the indents.
I would change this into:
confidence_interval_args (dict, optional (default=None)):
Parameters that will be fixed in the profile likelihood computation.
If None, all fittable parameters will be profiled except the poi.
Similar things apply to other docstrings complained about by reviewdog.
Pull Request Test Coverage Report for Build 5825140268
💛 - Coveralls |
Using best_fit_args is mostly relevant for 1D slices of higher-dimensional confidence volumes.
In most cases, we want it to be set to confidence_interval_args.
This PR sets it to confidence_interval_args, and makes the docstring more explicit. Also moves it to the last place in the argument list to emphasize that it is a rarer argument.
What do you think about setting confidence_interval_args via kwargs instead?