-
Notifications
You must be signed in to change notification settings - Fork 267
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 fit status fields to output container of MuonIntensityFitter #2381
Conversation
Added variable to check if fit converged, and storing of this variable in resulting container
Added new field to store an information about convergence of the muon intensity fit
Added assignment of new fields
Deleted unnecessary variable
Added commas
Could you add a test for the new fields? This could be simply checking
in one of the tests for now |
The only thing missing now is a changelog entry. You can add one by creating a file |
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.
Looks good! Thank you very much.
The pre-commit check is complaining about some style issues.
To prevent that in the future, you can locally run pre-commit install
After that, you can now run to fix it for this PR:
pre-commit run --files $(git diff origin/main --name-only)
Thank you for the explanation |
@jstvdk The correction for the style issues is still missing, that's all I guess. Need any help with fixing those? |
Yes please, I didn't understand this completely. |
The CI runs the same checks you can run locally. What is currently failing is the check for the style guide, which can be solved by auto-formatting the code using the pre-commit config and committing the result. Here you can see the error: To fix them, you can run:
And then add / commit the changes and push |
Thanks, I tried this solution, but it is not changing and pushing, because "Everything is up-to-date". |
Ah, sorry, I forgot that you are working on a fork. You need to compare to the main branch in this repository. Assuming you have it added as
To update and check:
Then run with upstream instead of origin:
|
Thank you, I made it. |
Ah, another issue is that your branch is behind the current main here in the repo, to fix that, you can do:
and then running the pre-commit command should work. |
@@ -89,6 +89,9 @@ def test_muon_efficiency_fit(prod5_lst): | |||
assert u.isclose(result.impact, impact_parameter, rtol=0.05) | |||
assert u.isclose(result.width, ring_width, rtol=0.05) | |||
assert u.isclose(result.optical_efficiency, efficiency, rtol=0.05) | |||
assert result.is_valid | |||
assert not result.parameters_at_limit | |||
assert np.isfinite(result.likelihood_value) |
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.
Is is very hard to change this test so it checks that the likelihood is reasonably close to a known value?
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.
Testing a particular value will be hard, yes. Maybe testing that the value is lower for a fit with the correct ring parameters passed in than for one where we don't have ring center / radius values?
Added field for the storing information about muon intensity fit convergence.
Related issue #2359