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

[ROOT6] Fix needed for root master to explicitly pass RooCmdArg #36903

Merged
merged 1 commit into from Feb 7, 2022

Conversation

smuzaffar
Copy link
Contributor

This is needed for ROOT master branch based IBs (CMSSW_12_3_ROOT6_X). Root change root-project/root@b553d38#diff-6534a03c2b46be206e3b5193cdad1ae96349308681da35a5b5cc54183aedcee3 was done e to make the implicit construction of RooCmdArg from just a string impossible . The change here explicitly use RooCmdArg("r", 0) to fix the ROOT6 build issue ( cms-sw/cmsdist#7601 (comment) )

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 7, 2022

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36903/28194

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 7, 2022

A new Pull Request was created by @smuzaffar (Malik Shahzad Muzaffar) for master.

It involves the following packages:

  • PhysicsTools/Utilities (analysis)

@cmsbuild, @santocch can you please review it and eventually sign? Thanks.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@smuzaffar
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 7, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c63559/22263/summary.html
COMMIT: 50c4148
CMSSW: CMSSW_12_3_X_2022-02-06-2300/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36903/22263/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 46
  • DQMHistoTests: Total histograms compared: 3766018
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3765994
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 45 files compared)
  • Checked 193 log files, 42 edm output root files, 46 DQM output files
  • TriggerResults: no differences found

@perrotta
Copy link
Contributor

perrotta commented Feb 7, 2022

+1

@perrotta
Copy link
Contributor

perrotta commented Feb 7, 2022

merge

@cmsbuild cmsbuild merged commit 2d33c7a into cms-sw:master Feb 7, 2022
@guitargeek
Copy link
Contributor

guitargeek commented Feb 7, 2022

Hi @smuzaffar, @perrotta , thanks for fixing this! But just to let you know, this is not the correct fix, even though it probably doesn't matter in this dead code.

I removed the implicit constructor of RooCmdArg from a string on purpose to avoid the creation of invalid command arguments, which I'll explain in the release notes: root-project/root#9833.

Now you found a way to still create the invalid command argument, that was not really the plan (thanks for nudging me to mention it in the release notes like this 😄) Rather, the compiler error hints to an error in the code, which you now reverted back back to a runtime error that will show up like this:

[#0] ERROR:InputArguments -- RooAbsPdf::fitTo(gauss) ERROR: unrecognized command: r

Probably not worth to change this again is this old class, just wanted to leave the paper trail here. The correct fix to also avoid runtime errors in the log would just be to remove the "r".

@perrotta
Copy link
Contributor

perrotta commented Feb 8, 2022

@guitargeek @smuzaffar apologize for having merged too quickly what looked to me like an innocuous fix.

@guitargeek , then what you suggest to implement instead is the following?

    fit_result = ModelPDF->fitTo(*Data);

If so, any of us should better commit this one line fix to clean up the release (even me, provided you confirm that this is actually the correct fix).

@smuzaffar
Copy link
Contributor Author

smuzaffar commented Feb 8, 2022

@perrotta #36907 contains the proper fix ( lets see if works for both root 6.24 and root master)

@perrotta
Copy link
Contributor

perrotta commented Feb 8, 2022

@perrotta #36907 contains the proper fix ( lets see if works for both root 6.24 and root master)

Thank you!

@guitargeek
Copy link
Contributor

Thanks at lot 👍

@santocch
Copy link

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will be automatically merged.

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.

None yet

5 participants