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

Enable choosing the fit strategy #182

Merged
merged 13 commits into from
Aug 7, 2024
Merged

Enable choosing the fit strategy #182

merged 13 commits into from
Aug 7, 2024

Conversation

hammannr
Copy link
Collaborator

What's the issue?

In the past, we observed that sometimes, and under certain conditions, minuit might not find a "valid" minimum for the fit. There are a few different solutions to this issue:

  • Continue the fit with higher precision
  • Use a different minimizer routine to begin with
  • Increase fit precision to begin with

What does this PR add?

In this PR, we enable choosing a fit strategy. For this, we add _fit_strategy attribute to the StatisticalModel class, which is used in the fit() and confidence_interval() method if not specified otherwise.
The _fit_strategy is a dictionary with the following entries:

Entry Functionality Options
minimizer_routine (Default: "migrad") the minimizer routine to use "migrad", "simplex", or "simplex_migrad" (first run simplex, then migrad)
minuit_strategy (Default: 1) "strategy" property of Minuit. The higher the number, the more precise the fit but also the slower. 0, 1, or 2
refit_invalid (Default: True) if True, refit with the simplex_migrad routine and strategy 2 if the optimization does not converge the first time. True/False
disable_index_fitting (Default: False) If True, disable the index fitting introduced in #156 even if the model has index parameters. True/False
max_index_fitting_iter (Default: 10) Maximum number of iterations for index fitting any integer

The last two are not new but were direct arguments of the fit method before.

There are a few ways to override the defaults on different levels:

  • Add fit_strategy with all/some overrides to your running config file and the strategy will be used for every computation of the runner
  • Initialize the statistical model with custom fit_strategy dict.
  • For an individual fit or confidence interval computation, pass a custom strategy to the method call. This can for example be used to compare performance and time requirements for different strategies before committing to one of them.

How does the default behavior change?

The only difference in the default behavior is that invalid minima are now refit by default, which was not possible before. So far, the Minuit strategy was not specified and the Minuit default of 1 was used, which is the default also now. Also, the standard minimizer routine remains migrad as before.

@hammannr hammannr added enhancement New feature or request inference Statistics code labels Jul 30, 2024
@hammannr hammannr requested review from kdund, zihaoxu98 and dachengx July 30, 2024 16:00
@coveralls
Copy link

coveralls commented Jul 30, 2024

Pull Request Test Coverage Report for Build 10288140451

Details

  • 34 of 39 (87.18%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.1%) to 91.469%

Changes Missing Coverage Covered Lines Changed/Added Lines %
alea/model.py 31 36 86.11%
Totals Coverage Status
Change from base Build 10279089356: -0.1%
Covered Lines: 1662
Relevant Lines: 1817

💛 - Coveralls

dachengx
dachengx previously approved these changes Aug 2, 2024
Copy link
Collaborator

@dachengx dachengx left a comment

Choose a reason for hiding this comment

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

Looks good. But then you also need to update the OSG submitter.

@hammannr
Copy link
Collaborator Author

hammannr commented Aug 2, 2024

Looks good. But then you also need to update the OSG submitter.

@dachengx I only modified the submitter base class since all other submitters will parse additional arguments to the base class or am I missing something? Unfortunately, I can't test this behaviour though.

@hammannr
Copy link
Collaborator Author

hammannr commented Aug 6, 2024

@dachengx at least on midway this seems to be working fine. Any reason why it shouldn't work for the osg submitter? Maybe also @FaroutYLq can have a quick look 😊

@FaroutYLq
Copy link
Contributor

@hammannr I think it should work for OSG, since in principle the new fit_strategy things should just show up in the ticket generator outputs, and collected here. I didn't do test though (and it is nontrivial to test on OSG until this alea is in environment)

@hammannr
Copy link
Collaborator Author

hammannr commented Aug 7, 2024

After some internal discussions, we decided that the standard behavior should be that the strategy is defined in the config file of the statistical model and that it can be overwritten by values in the runner. I added this functionality now i.e. runner fit strategy > stat. model fit strategy > default fit strategy.

@hammannr hammannr merged commit aac82c6 into main Aug 7, 2024
7 checks passed
@hammannr hammannr deleted the minimizer_routines branch August 7, 2024 16:34
@hammannr hammannr mentioned this pull request Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request inference Statistics code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants