-
Notifications
You must be signed in to change notification settings - Fork 393
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
Random start points option for profiled POIs #713
Random start points option for profiled POIs #713
Conversation
I start running some test. The automatic ones run w/o this option on, but I think it is still a useful thing to assess. |
Pull Request Test. |
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.
Thanks for the PR - a few comments to start with. I'm sure others also have comments (e.g. I think there was a suggestion to move this algorithm to a separate class that is simply called in MultiDimFit, rather than it being embedded in MDF as it is now).
src/MultiDimFit.cc
Outdated
unsigned int points = pointsPerPoi.size() == 0 ? points_ : pointsPerPoi[0]; | ||
|
||
// Set seed for random points | ||
srand(1); |
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.
Not sure right now if this could have other knock-on effects, but perhaps it would be better to do this only if pointsRandProf > 1
just to be safe
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.
Resolved.
src/MultiDimFit.cc
Outdated
@@ -665,42 +680,148 @@ void MultiDimFit::doGrid(RooWorkspace *w, RooAbsReal &nll) | |||
} | |||
|
|||
//if (verbose > 1) std::cout << "Point " << i << "/" << points << " " << poiVars_[0]->GetName() << " = " << x << std::endl; | |||
std::cout << "Point " << i << "/" << points << " " << poiVars_[0]->GetName() << " = " << x << std::endl; | |||
std::cout << "Point " << i << "/" << points << " " << poiVars_[0]->GetName() << " = " << x << std::endl; | |||
*params = snap; |
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.
From here to L788 or so I think this should also be wrapped in an if (pointsRandProf > 1)
statement, to avoid this being invoked when the option is not explicitly called.
Here I think it would also good to print a warning / log message to state that this option is active (hopefully avoids people playing with random options and being surprised the fit is taking much longer than before)
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.
There is a certain caveat to this suggestion. In our current implementation, the default behavior (i.e. just using the default starting point) is incorporated within the random starting point code block. If one wants to retrieve this behavior, one can either choose to set pointsRandProf
to 0 or just entirely omit the option while running combine MultiDimFit. The reason why we chose to have the implementation set up this way is to avoid a lot of code repetition. In order to make sure that the users do not get confused about this, we have added some comment blocks in MultiDimFit method stating what I just explained above. Also, there are some printout code blocks that tell the user when pointsRandProf
is set to a non-zero value (and also when the option is omitted). Let us know what you think about this approach.
src/MultiDimFit.cc
Outdated
std::vector<float> nll_at_alt_start_pts; | ||
|
||
// Get vector of points to try | ||
float prof_start_pt_range_max = 20.0; // Is this what we want? |
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.
For a generic version of this algorithm, I think you'd want to potentially have a different range for each of the POIs, maybe the range can be read from the ws for each POI? (or the range set by --setParameterRanges)
src/MultiDimFit.cc
Outdated
if (verbose > 1) std::cout << "\n\tStart pt idx: " << start_pt_idx << std::endl; | ||
for (unsigned int var_idx=0; var_idx<specifiedVars_.size(); var_idx++){ | ||
if (verbose > 1) std::cout << "\t\tThe var name: " << specifiedVars_[var_idx]->GetName() << std::endl; | ||
if (strcmp(specifiedVars_[var_idx]->GetName(),"r")==0) { |
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.
Should this be skipped in general?
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.
This block of skipping "r" has been removed. We agreed that there is no need to skip this in general.
src/MultiDimFit.cc
Outdated
for(unsigned int j=0; j<specifiedFuncNames_.size(); j++){ | ||
specifiedFuncVals_[j]=specifiedFunc_[j]->getVal(); | ||
} | ||
for(unsigned int j=0; j<specifiedNuis_.size(); j++){ |
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.
Restore original spacing?
src/MultiDimFit.cc
Outdated
for(unsigned int j=0; j<specifiedCatNames_.size(); j++){ | ||
specifiedCatVals_[j]=specifiedCat_[j]->getIndex(); | ||
} | ||
for(unsigned int j=0; j<specifiedNuis_.size(); j++){ |
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.
Restore original spacing?
src/MultiDimFit.cc
Outdated
Combine::commitPoint(true, /*quantile=*/0); | ||
continue; | ||
} | ||
ok = fastScan_ || (hasMaxDeltaNLLForProf_ && (nll.getVal() - nll0) > maxDeltaNLLForProf_) || utils::countFloating(*params)==0 ? |
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.
Just from looking at this I think this instance of ok
is not used at all as L803 is always invoked afterwards, thus no need to set it here?
As mentioned above, if it's not too difficult, it could be better to put this code into a new class in a separate .cc file, which could then be called by MultiDimFit when the extra command line option is added. MultiDimFit is already due a clean up in the near future, and I think this would make things easier to maintain. |
Thank you @adewit and @ajgilbert for the reviews. We're working on addressing the comments, and will get back to you as soon as possible. However, we're a bit swamped with work on other areas of our analysis right now, so it might take us a few weeks. But if this timeframe would present an issue for eventually moving forward with merging this PR (e.g. if other large changes to this section of the code are expected soon), please let us know and we attempt to get it done more quickly. Thanks! |
Hi @kmohrman, no problem from our side I think - we're usually pretty slow getting our own developments in. Will let you know if/when a big refactor of MultiDimFit is imminent. |
Hi, thanks again for the reviews of this PR. Sorry for our long silence, things have been very busy with other parts of our analysis. We're now pushing through the final stages, and have run into a few things we wanted to add to this PR in order to make this random starting point method work better for our analysis. Anyway, just wanted to give a heads up that we'll be pushing a few changes to this branch, and wanted to say sorry in advance for the noise! |
Pull Request Test. |
Pull Request Test. |
Pull Request Test. |
Pull Request Test. |
Pull Request Test. |
Include default for "noData" option (avoid complaint when running with --t2w)
Hi @ajgilbert, my apologies for a delayed response to this. At this point, I have resolved the merge conflicts that arose after we switched the target branch to
|
Hi @ajgilbert, sorry for the noise. I was wondering what the status of the review is. Let me know if you have had a chance to work on this and if you have any comments. |
I tried compiling under CMSSW_11_3_4 and i'm hitting compilation errors at src/MultiDimFit.cc:712 . Unfortunately, the automatic checks are using CMSSW_10_2 which seem to pass. @abasnet97 , can you confirm which CMSSW version you have been testing your PR with? |
@nucleosynthesis, looks like I am using CMSSW_10_2_13. |
Hi @nucleosynthesis , I wanted to follow-up to see if there has been any development on this front. Were there some issues with cmssw release mismatch? |
Hi Again, apologies for the delay in responding. @abasnet97 - can you try to compile under CMSSW_11_3_4 and see if there is a way to fix the errors? Let us know if you get stuck All the best, |
Hi @nucleosynthesis, after fixing some errors, I can now compile the repo using CMSSW_11_3_4. I have also merged all recent changes from main to this branch. |
Just adding @anigamova and @kcormi here to keep CAT-stats in the loop. |
I think this looks ok, but I'd really like to test it. @abasnet97 - can you provide an example command that will work with one of our tutorial datacards? For example, if you try the following,
When I try the above, I don't see the multiple fits being performed that I would have expected - can you try and see if there's edits to the command line that would make it work? Nick |
Hi Nick, your |
Hi @abasnet97 , Right of course, I missed that the grid algo is needed since this is only intended for scans (though I wonder why this wouldn't also be useful for the global fit part to make sure the correct global minimum is found) Anyway I'll try with the --grid added, Thanks |
Hi Nick, |
Ok, adding the |
Hi @nucleosynthesis, a quick question. When you say:
What command line option are you talking about? |
Here is the command I tried, where I was seeing the errors
(note, as per my earlier comment, you can create this workspace via
) |
Ok let me try this quick. |
Hi @nucleosynthesis, so I tested your command. And yes, I did see those warnings.
I saw significantly less number of warnings. Now, what range to pick for these random start points are of course specific to each parameters, so my dummy values might not have been the best range. |
Thanks for checking - I think its not great to have a separate option to set these ranges, nor to use -20,20 as the default (what if the POI was a mass variable for example). Given most physics model define sensible default ranges, I would remove the |
Hi @nucleosynthesis, thank you for the comments. I agree that your suggestion would be a possibility. However, in some cases (e.g. TOP-22-006, which was the use case this PR was developed for), we believe it is useful to have a separate option for specifying the ranges from which to draw the random points. I think the following reasons contribute to the usefulness of the
For these reasons, we would prefer to keep the Thanks! |
The point is that the ranges for the parameters are already modifiable in the command line using
either of the above will just cause confusion and moreover will spoil setting the ranges used in the fit for the parameter. Unless there is a specific reason to have a different range for the random starting point vs that allowed in the fit, I really think we need to avoid introducing this extra option. At the very least, if the user doesn't specify Finally, if we really need this extra option, can we change it to something like |
After thinking about and discussing your points, we agree that a clash between To this end, we would propose the following:
We are wondering if you would have thoughts on this proposal? |
This sounds ok to me, but please consider changing the name of the option to something more informative than |
That name ( |
Hi @nucleosynthesis, I have implemented the changes that @kmohrman described in this comment. And like you suggested, I have also edited the name of the CLI option to be a bit less confusing. |
Remove unecessary COUT and add logger command
This PR adds an option
--pointsRandProf
toMultiDimFit
.n==1
case of thedoGrid()
function. If the option can be disabled by either not specifying the option, or by specifying 0 for the number of points.combine
experts would have.Thank you!