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

surprising behavior from Analysis.fit_hypo #661

Open
atfienberg opened this issue Mar 15, 2021 · 3 comments
Open

surprising behavior from Analysis.fit_hypo #661

atfienberg opened this issue Mar 15, 2021 · 3 comments

Comments

@atfienberg
Copy link
Contributor

After setting a distribution maker's parameters to new Param objects, fit_hypo may discard these parameters before fitting. One example of how this could occur is this:

  1. Conduct a fit
  2. Set the distribution maker's parameters to copy.deepcopy(best_fit_params)
  3. Evaluate the distribution at the best fit parameters
  4. Fix some parameters to new values
  5. Run another fit

Under some conditions, the fit in step 5. will discard the parameters from step 4. This occurs because of the call to hypo_maker.select_params(full_param_selections). I found this behavior very surprising. Whether the fit actually uses parameters fixed before the call to fit_hypo depends on whether the user modifies parameters in place (which might work) or sets the parameters to new Param objects (which probably won't work),

@philippeller
Copy link
Collaborator

Related to #644

@mvprado0
Copy link
Contributor

mvprado0 commented Aug 31, 2021

I am finding the same problem, and with the current Pisa functions for ParamSet, there doesn't seem to be a way to make the current analysis.py code use best fit parameter starting values and set them free at the same time. All one can do if you want to keep the best fit values is fix the parameters in place as described above. A quick and simple way to fix this without messing with select_params() or ParamSet would be with a simple flag in fit_hypo that incorporates an if statement into the line hypo_maker.select_params(full_param_selections).

Something like:

def fit_hypo(self, data_dist, hypo_maker, metric, minimizer_settings, using_previous_params=False, ...):
if using_previous_params==False:
hypo_maker.select_params(full_param_selections)

If this is okay, I would be happy to make a PR for it.

@LeanderFischer
Copy link
Collaborator

LeanderFischer commented Oct 6, 2021

I come across the same issue while running systematic impact tests with the high stats standard numu disappearance neutrino pipeline. Without setting recursive_minimization=True, fit_hypo is being called for every fit and for the null fits, where the physics parameters were fixed just before calling the fit, they are freed again by re-selecting the parameters, which is quite contrary to what is wanted. If instead i specify that i want the recursive minimization it directly calls fit_recursively, here, and everything is fine. Is it really necessary to re-select the parameters in fit_hypo? Not sure how other analyses are set up, but i feel for most of them the selection was already made beforehand and the hypo_maker that is passed to the fit function is using the correct selection.

P.s.: Just for my understanding, when hypo_maker.select_params is called, where does it access the original selection parameters? Should they not have been changed with the changes of hypo_maker.params? Sorry, if i'm being dense, but i'm just diving back into the intricacies of Pisa 😅

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

No branches or pull requests

4 participants