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 optional argument degree_of_freedom for asymptotic_critical_value #86

Merged
merged 7 commits into from
Sep 18, 2023

Conversation

dachengx
Copy link
Collaborator

@dachengx dachengx commented Sep 3, 2023

Resolve: #83

What does the code in this PR do / what does it improve?

Add optional argument degree_of_freedom for asymptotic_critical_value

Can you briefly describe how it works?

  1. For models, if asymptotic_dof is provided as an __init__ argument, the StatisticalModel.asymptotic_dof will be used in _confidence_interval_checks.
  2. For runners, if asymptotic_dof is in statistical_model_args, the Runner.confidence_interval_thresholds will be set according to asymptotic_dof, and further used in CL calculation.

Can you give a minimal working example (or illustrate with a figure)?

from alea.utils import asymptotic_critical_value
asymptotic_critical_value("central", 0.9)
>>> 2.705543454095414

What are the potential drawbacks of the codes?

When "confidence_interval_kind" is not "lower" or "upper", and degree_of_freedom is not None or 1, asymptotic_critical_value will raise an error.

Please include the following if applicable:

  • Update the docstring(s)
  • Update the documentation
  • Tests to check the (new) code is working as desired.
  • Does it solve one of the open issues on github?

@github-actions
Copy link

github-actions bot commented Sep 3, 2023

Pull Request Test Coverage Report for Build 6223968781

  • 10 of 16 (62.5%) changed or added relevant lines in 3 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.4%) to 91.81%

Changes Missing Coverage Covered Lines Changed/Added Lines %
alea/submitter.py 1 2 50.0%
alea/utils.py 4 5 80.0%
alea/model.py 5 9 55.56%
Files with Coverage Reduction New Missed Lines %
alea/model.py 1 86.1%
Totals Coverage Status
Change from base Build 6052911707: -0.4%
Covered Lines: 1334
Relevant Lines: 1453

💛 - Coveralls

Copy link
Collaborator

@kdund kdund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change the None defaults to 1 for the DOF?

alea/model.py Outdated Show resolved Hide resolved
@@ -91,7 +91,7 @@ def data(self, data: Union[dict, list]):
If data is a list, it must be a list of length len(self.likelihood_names) + 1.

Raises:
ValueError: If data is not a list of length len(self.likelihood_names) + 1.
Warning: If data is not a list of length len(self.likelihood_names) + 1.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why warning and not error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There will be no generate_values in real data, so we will set a set of data without generate_values. This was introduced along with https://github.com/XENONnT/alea/pull/79/files#diff-ccb14fbc2f6819c7774476fe82d0c97563c7006391df64c86b0fbea0706af8f5R421 store_real_data function.

alea/utils.py Show resolved Hide resolved
@dachengx
Copy link
Collaborator Author

I need to submit a new PR to solve the deprecated readthedocs configuration. https://readthedocs.org/projects/alea/builds/21954754/

kdund
kdund previously approved these changes Sep 18, 2023
Copy link
Collaborator

@kdund kdund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @dachengx !

@dachengx
Copy link
Collaborator Author

dachengx commented Sep 18, 2023

Hey, @kdund . Sorry for the back-and-forth, I was validating the PR by non-beta study. I find that it is better to set asymptotic_dof of StatisticalModel.confidence_interval to None to avoid mis-assignment. But still the default asymptotic_dof of StatisticalModel.__init__ is 1. I think this will ease most of the pain, and one does not need to set it explicitly as 1 in most cases.

So though the default value of asymptotic_dof of StatisticalModel.confidence_interval is None, the asymptotic_dof can be readout from StatisticalModel.asymptotic_dof.

@dachengx dachengx merged commit 278932c into main Sep 18, 2023
4 checks passed
@dachengx dachengx deleted the asymp_n_dof branch September 18, 2023 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add degree of freedom to asymptotic_critical_value
2 participants