Skip to content

[RF] Delete fit objects before continuing in rs401d_FeldmanCousins.py #22853

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.

Sign up for GitHub

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

Open
guitargeek wants to merge 1 commit into
base: master
Choose a base branch
from
+6 −0
Open

[RF] Delete fit objects before continuing in rs401d_FeldmanCousins.py #22853

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions tutorials/roofit/roostats/rs401d_FeldmanCousins.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,12 @@
dataCanvas.Draw()
dataCanvas.SaveAs("3.png")

# The profile likelihood and the NLL are not needed anymore. Delete them
# already here, so that the NLL evaluation machinery is not torn down at
# interpreter shutdown, where the order of cleanups is less controlled.
del pll
del nll
Comment on lines +167 to +171

@vepadulano vepadulano Jul 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not to block this PR, but in general it's weird/a code smell to have to use del in Python at determined places. Do we have an idea of how this particular scenario could be fixed with e.g. appropriate Pythonizations or changes in the bindings layer?

@guitargeek guitargeek Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's hard to investigate, because the crash only happens in the cppyy upgrade PR. I guess some different destruction ordering there turns out to be catastrophic for RooFit.

My plan was to first make sure that this works for old and new cppyy by making the destruction order more deterministic with this PR, and then at a later point revisit the dels in the tutorial when there is time.


# --------------------------------------------------------------
# show use of Feldman-Cousins utility in RooStats
# set the distribution creator, which encodes the test statistic
Expand Down Expand Up @@ -230,7 +236,7 @@
forContour = hist.Clone()

# now loop through the points and put a marker if it's in the interval
tmpPoint = RooArgSet()

Check failure on line 239 in tutorials/roofit/roostats/rs401d_FeldmanCousins.py

View workflow job for this annotation

GitHub Actions / ruff

ruff (F821) 

tutorials/roofit/roostats/rs401d_FeldmanCousins.py:239:20: F821 Undefined name `RooArgSet`
# loop over points to test
for i in range(parameterScan.numEntries):
# get a parameter point from the list of points to test.
Expand All @@ -257,7 +263,7 @@

mcPlot = ROOT.nullptr
if mcInt:
print(f"MCMC actual confidence level: ", mcInt.GetActualConfidenceLevel())

Check failure on line 266 in tutorials/roofit/roostats/rs401d_FeldmanCousins.py

View workflow job for this annotation

GitHub Actions / ruff

ruff (F541) 

tutorials/roofit/roostats/rs401d_FeldmanCousins.py:266:15: F541 f-string without any placeholders
  help: Remove extraneous `f` prefix
mcPlot = ROOT.RooStats.MCMCIntervalPlot(mcInt)
mcPlot.SetLineColor(ROOT.kMagenta)
mcPlot.Draw()
Expand Down
Loading