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

Harmonize docstring for test_stat between AsymptoticCalculator and ToyCalculator #1335

Closed
matthewfeickert opened this issue Feb 22, 2021 · 6 comments · Fixed by #1970
Closed
Labels
docs Documentation related good first issue Good for newcomers

Comments

@matthewfeickert
Copy link
Member

Description

In v0.6.0 docs, the AsymptoticCalculator API has a nicely formatted docstring for the test_stat arg

test_stat (:obj:`str`): The test statistic to use as a numerical summary of the
data: ``'qtilde'``, ``'q'``, or ``'q0'``.
* ``'qtilde'``: (default) performs the calculation using the alternative test statistic,
:math:`\tilde{q}_{\mu}`, as defined under the Wald approximation in Equation (62)
of :xref:`arXiv:1007.1727` (:func:`~pyhf.infer.test_statistics.qmu_tilde`).
* ``'q'``: performs the calculation using the test statistic :math:`q_{\mu}`
(:func:`~pyhf.infer.test_statistics.qmu`).
* ``'q0'``: performs the calculation using the discovery test statistic
:math:`q_{0}` (:func:`~pyhf.infer.test_statistics.q0`).

but the ToyCalculator API didn't have these changes applied to them (seems @matthewfeickert forgot to propagate these in PR #993)

test_stat (:obj:`str`): The test statistic to use as a numerical summary of the
data: ``'qtilde'``, ``'q'``, or ``'q0'``.
``'qtilde'`` (default) performs the calculation using the alternative test statistic,
:math:`\tilde{q}_{\mu}`, as defined under the Wald approximation in Equation (62)
of :xref:`arXiv:1007.1727` (:func:`~pyhf.infer.test_statistics.qmu_tilde`), ``'q'``
performs the calculation using the test statistic :math:`q_{\mu}`
(:func:`~pyhf.infer.test_statistics.qmu`), and ``'q0'`` perfoms the calculation using
the discovery test statistic :math:`q_{0}` (:func:`~pyhf.infer.test_statistics.q0`).

The ToyCalculator docstring should be updated to match.

@matthewfeickert matthewfeickert added good first issue Good for newcomers docs Documentation related labels Feb 22, 2021
@kratsg
Copy link
Contributor

kratsg commented Feb 22, 2021

Similarly, need to update the warning for qmu to be using qmu_tilde with the more appropriate suggestion.

@Saransh-cpp
Copy link
Member

@matthewfeickert can I take this up? @kratsg could you please elaborate a bit more on the warning updates?

@Saransh-cpp
Copy link
Member

@matthewfeickert @kratsg not to disturb you again, but any updates on this?

@kratsg
Copy link
Contributor

kratsg commented Sep 7, 2021

The warning updates were already done. As for documentation, why not just submit the PR? But unless you know the statistics content, we'd recommend not just rewriting without knowing what the content itself is.

@matthewfeickert
Copy link
Member Author

But unless you know the statistics content, we'd recommend not just rewriting without knowing what the content itself is.

Yeah, I was envisioning a small rewrite of this when I opened the Issue. My advice would be to only work on a PR for this if you feel comfortable with the frequentist statistics being discussed, else we'll probably have to request multiple revisions.

@Saransh-cpp
Copy link
Member

Oh oh, my bad, I thought this required only formatting of the documentation. Thanks for letting me know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation related good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants