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

Use likelihood fit instead of chi-square fit in DQMServices slice fits #43106

Merged
merged 1 commit into from
Nov 1, 2023
Merged

Use likelihood fit instead of chi-square fit in DQMServices slice fits #43106

merged 1 commit into from
Nov 1, 2023

Conversation

guitargeek
Copy link
Contributor

@guitargeek guitargeek commented Oct 24, 2023

The DQM plots use the TH2::FitSlicesY() function to fit some Gaussians. However, some of the fits are failing. This was not resulting in errors so far, but with the switch to Minuit2 by default in ROOT 6.30 it will.

The problem is that it uses chi-square fits to fit slices with many empty bins, which is not appropriate. Doing a likelihood fit with the "l" option is one way to fix the problem, because it can better deal with empty bins.

Thanks to @lmoneta for this suggestion!
See root-project/root#13852

Closes #42979.

@smuzaffar, needs to be tested with ROOT master or 6.30 if possible.

The DQM plots use the `TH2::FitSlicesY()` function to fit some Gaussians.
However, some of the fits are failing. This was not resulting in errors
so far, but with the switch to Minuit2 by default in ROOT 6.30 it will.

The problem is that it uses chi-square fits to fit slices with many
empty bins, which is not appropriate. Doing a likelihood fit with the
`"l"` option is one way to fix the problem, because it can better deal
with empty bins.

Closes #42979.
@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43106/37349

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 24, 2023

A new Pull Request was created by @guitargeek (Jonas Rembser) for master.

It involves the following packages:

  • DQMServices/Components (dqm)

@tjavaid, @syuvivida, @rvenditti, @nothingface0, @cmsbuild, @antoniovagnerini can you please review it and eventually sign? Thanks.
@barvic this is something you requested to watch as well.
@sextonkennedy, @antoniovilela, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@smuzaffar
Copy link
Contributor

test parameters:

  • workflow = 4.63,21.0,24.0,36.0,43.0,250409.0

@smuzaffar
Copy link
Contributor

please test for CMSSW_13_3_ROOT630_X

@smuzaffar
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-3e8a50/35411/summary.html
COMMIT: dfced23
CMSSW: CMSSW_13_3_X_2023-10-24-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/43106/35411/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 66 lines to the logs
  • Reco comparison results: 29 differences found in the comparisons
  • DQMHistoTests: Total files compared: 55
  • DQMHistoTests: Total histograms compared: 3789147
  • DQMHistoTests: Total failures: 61906
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3727209
  • DQMHistoTests: Total skipped: 32
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 54 files compared)
  • Checked 236 log files, 187 edm output root files, 55 DQM output files
  • TriggerResults: no differences found

@smuzaffar
Copy link
Contributor

please test

There are too many comparison differences, so lets re-run based on latest IB

@guitargeek
Copy link
Contributor Author

I would not be surprised if there are real differences, given that the DQM Histograms are now fit without the chi-square approximation

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-3e8a50/35417/summary.html
COMMIT: dfced23
CMSSW: CMSSW_13_3_X_2023-10-25-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/43106/35417/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 1 lines from the logs
  • Reco comparison results: 22 differences found in the comparisons
  • DQMHistoTests: Total files compared: 56
  • DQMHistoTests: Total histograms compared: 3795089
  • DQMHistoTests: Total failures: 61805
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3733252
  • DQMHistoTests: Total skipped: 32
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 55 files compared)
  • Checked 237 log files, 188 edm output root files, 56 DQM output files
  • TriggerResults: no differences found

@mmusich
Copy link
Contributor

mmusich commented Oct 26, 2023

I would not be surprised if there are real differences, given that the DQM Histograms are now fit without the chi-square approximation

by naively looking into some of the histograms changed (e.g. https://tinyurl.com/yo6kah8c or https://tinyurl.com/ywu9hhm2) the fit quality looks better now.

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-3e8a50/35409/summary.html
COMMIT: dfced23
CMSSW: CMSSW_13_3_ROOT630_X_2023-10-24-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/43106/35409/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 61 lines to the logs
  • Reco comparison results: 35 differences found in the comparisons
  • DQMHistoTests: Total files compared: 55
  • DQMHistoTests: Total histograms compared: 3789147
  • DQMHistoTests: Total failures: 63259
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3725856
  • DQMHistoTests: Total skipped: 32
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 54 files compared)
  • Checked 236 log files, 187 edm output root files, 55 DQM output files
  • TriggerResults: no differences found

@smuzaffar
Copy link
Contributor

@cms-sw/dqm-l2 can you please review it. This fixes the Relvals failures for ROOT 6.28/30 IBs

@smuzaffar
Copy link
Contributor

@rappoccio @antoniovilela can we merge this for 11h00 IB so thatit can go in ROOT 6.28/30 builds too

@antoniovilela
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit da3c6d2 into cms-sw:master Nov 1, 2023
18 checks passed
@antoniovilela
Copy link
Contributor

@rappoccio @antoniovilela can we merge this for 11h00 IB so thatit can go in ROOT 6.28/30 builds too

@smuzaffar
Hi Shahzad,
Try to ping us using @cms-sw/orp-l2. I seem to pick it up faster.
Thanks,

@AdrianoDee
Copy link
Contributor

For the records this PR is the cause of some failures in the 13_3_0_pre5 given the worsening of track/muons resolutions. E.g.:

@guitargeek
Copy link
Contributor Author

Can you maybe post some plots here for non-CMS members like me? 🙂

@davidlange6
Copy link
Contributor

one example is this one - the fit is doing what it should (red chi2, green likelihood) - however, I suspect the intent is to estimated a resolution while ignoring the tails (which is not what the code does..)

image

@guitargeek
Copy link
Contributor Author

Thanks David! Indeed neither fit looks appropriate here, maybe better just take the std. dev from the histogram or as as you said fit only the central region.

@AdrianoDee
Copy link
Contributor

Yes, it's more a problem related to the way we calculate the resolution itself rather than directly originating from this PR (that basically highlights the issue).

Still one could argue that red is still better than green since the likelihood gives more importance to the tails, in the specific case posted here and elsewhere. Therefore the failure reports.

@guitargeek
Copy link
Contributor Author

Relying on accidental properties of the chi2 fit is quite a random way to discount the tails. @AdrianoDee, what to you thing about Davids suggestion to only fit the central region (which could then be done with either chi2 or likelihood)?

@mmusich
Copy link
Contributor

mmusich commented Jan 9, 2024

what to you thing about Davids suggestion to only fit the central region (which could then be done with either chi2 or likelihood)?

chiming in my (unrequested) 2 cents:

  • one of the points that we should not forget is that DQMGenericClient (as the name sorts of betrays) was originally devised as a generic tool to perform gaussian fits in a variety of DQM harvesting applications. Thanks to this PR we have now (re-?) discovered that this tool is used sometimes inappropriately to fit (also) non-gaussian distributions (e.g. residuals). On the other hand one might assume that the bulk of the use cases is using it appropriately (e.g. pulls), so restricting the fit to the central region might be erring on the opposite side.
  • all in all it is perhaps better to let to each one of the clients to choose in which modality to perform the fit (full range, limited range, not fitting at all, etc.) ?

@makortel
Copy link
Contributor

How should we proceed here?

Does the <= 13_3_0_pre4 behavior need to be restored by 14_0_0_pre3 (last open prerelease)?

If yes, are the improvements in DQMGenericClient (along what @mmusich outlined in #43106 (comment)) something that could be developed, tested, and deployed in that time frame?

I guess, in principle, a possible "quick fix" would be to revert to ROOT 6.26 and the chi2 based fitting (while the improvements are being worked on), but I see that only as the last resort.

@mmusich
Copy link
Contributor

mmusich commented Jan 16, 2024

if yes, are the improvements in DQMGenericClient (along what @mmusich outlined in #43106 (comment)) something that could be developed, tested, and deployed in that time frame?

there seems to be already something that does limited range fits:

limitedFit(srcME, meanME, sigmaME);

perhaps it's just a matter of adjusting configurations. @cms-sw/tracking-pog-l2 @cms-sw/muon-pog-l2 for your consideration.

@smuzaffar
Copy link
Contributor

@guitargeek , during offline release planning meeting today, we discussed if it is possible to use the old Minuit (instead of new default Minit2 ). Do you know if it is possible and how?

@guitargeek
Copy link
Contributor Author

In which scope? All of CMSSW?

@smuzaffar
Copy link
Contributor

yes all cmssw ( e.g. may be build root to use old Minuit as default)

@makortel
Copy link
Contributor

From root-project/root#13661 and root-project/root#13852 it seem like

ROOT::Math::MinimizerOptions::SetDefaultMinimizer("Minuit");
// or
ROOT::Math::MinimizerOptions::SetDefaultMinimizer("Minuit", "Migrad");

would do the job. I suppose this function is thread-unsafe, so maybe the InitRootHandlers constructor would be a reasonable place to call it (especially given that this should be only a temporary workaround).

Then in the meanwhile @cms-sw/dqm-l2 @cms-sw/tracking-pog-l2 @cms-sw/muon-pog-l2 could look into restricting the fit ranges along #43106 (comment).

After that we could try again to move the DQMGenericClient (and others) to likelihood fits, after which we could move to Minuit2 by default (and all this while staying in Root 6.30). And probably now the discussion should be moved to a new issue.

@makortel
Copy link
Contributor

I suppose this function is thread-unsafe

I found these

void CalibrationAlgorithm::analyse() {
ROOT::Math::MinimizerOptions::SetDefaultMinimizer("Minuit2", "Migrad");
ROOT::Math::MinimizerOptions::SetDefaultStrategy(0);

void CalibrationScanAlgorithm::analyse() {
ROOT::Math::MinimizerOptions::SetDefaultMinimizer("Minuit2", "Migrad");
ROOT::Math::MinimizerOptions::SetDefaultStrategy(0);

I guess (hope) these components are not run in standard workflows.

@mmusich
Copy link
Contributor

mmusich commented Jan 16, 2024

I guess (hope) these components are not run in standard workflows.

IIRC, indeed they aren't. @rgerosa might confirm

@rgerosa
Copy link
Contributor

rgerosa commented Jan 16, 2024

Hi @mmusich yes I confirm these are just run in the analysis of tracker local-runs of type calibration scan

@makortel
Copy link
Contributor

And probably now the discussion should be moved to a new issue.

The issue is here #43722 . I suggest to move all subsequent discussion on the topic there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CMSSW tests fails with Fatal Root Error: @SUB=Minuit2
10 participants