Skip to content

Commit 2ef2859

Browse files
committed
[Python] Deprecate conversion from Python set to RooArgSet
This conversion can lead to subtle problems because Python sets are unordered, different from C++ initializer lists with the same syntax. This can lead to confusion that we should prevent. Example: ```python import ROOT x = ROOT.RooRealVar("x", "x", 1.0) y = ROOT.RooRealVar("y", "y", -1.0) ROOT.RooArgSet(set([x, y])).Print() ROOT.RooArgSet(set([y, x])).Print() ``` Gives on my system: ```txt RooArgSet:: = (x,y) RooArgSet:: = (x,y) <--- reverse order expected! ``` This is not even deterministic, because Python doesn't guarantee that a `set` is ordered in any way. I introduced this 5 years ago in commit c54c324 (PR #8751) whithout thinking too much about this footgun.
1 parent 9916067 commit 2ef2859

3 files changed

Lines changed: 22 additions & 3 deletions

File tree

README/ReleaseNotes/v642/index.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ The following people have contributed to this new version:
4747
* The `TSSLSocket` class is now removed following deprecation in ROOT 6.40.
4848
* The bindings to the R programming language that are enabled with the `r=ON` or `tmva-rmva=ON` build options (`TRInterface`, RMVA, and friends) are removed, following deprecation in ROOT 6.40. Their maintenance is no longer justified, given the broader adoption of the scientific Python ecosystem. Users who still rely on R from C++ are encouraged to call R directly via https://cran.r-project.org/package=RInside, which is what the ROOT bindings were using internally.
4949
* Several enums that are redundant with `ROOT::ESTLType` are deprecated and will be removed in ROOT 6.44: `TClassEdit::ESTLType`, `TDictionary::ESTLType`, `TStreamerElement::ESTLType`. Please use `ROOT::ESTLType` instead.
50+
* The conversion from Python set to **RooArgSet** is deprecated and won't work anymore in ROOT 6.44. The problem is that Python sets are unordered while RooArgSets are ordered, and this mismatch can lead to subtle problems later on. Prefer conversion from Python lists or tuples, which are ordered too.
5051

5152
## Python Interface
5253

bindings/pyroot/pythonizations/python/ROOT/_pythonization/_roofit/_rooargset.py

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,13 +11,13 @@
1111

1212

1313
import operator
14+
import warnings
1415

1516
from ._rooabscollection import RooAbsCollection
1617

1718

1819
class RooArgSet(RooAbsCollection):
19-
20-
__cpp_name__ = 'RooArgSet'
20+
__cpp_name__ = "RooArgSet"
2121

2222
def __init__(self, *args, **kwargs):
2323
"""Pythonization of RooArgSet constructor to support implicit
@@ -30,6 +30,16 @@ def __init__(self, *args, **kwargs):
3030
# argument is added, hence the `len(kwargs) == 0` check breaks the
3131
# implicit conversion!
3232
if len(args) == 1 and isinstance(args[0], set):
33+
warnings.warn(
34+
"Constructing a RooArgSet from a Python set is deprecated and "
35+
"will be removed in ROOT 6.44. Python sets are unordered, while "
36+
"a RooArgSet is ordered, so this conversion can silently change "
37+
"the order of the elements. Construct the RooArgSet from the "
38+
"individual RooFit objects, or from a list or tuple of them "
39+
"instead.",
40+
FutureWarning,
41+
stacklevel=2,
42+
)
3343
return self._init(*args[0], **kwargs)
3444
return self._init(*args, **kwargs)
3545

bindings/pyroot/pythonizations/test/root_module.py

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,12 +120,20 @@ def test_deprecated_features(self):
120120

121121
version = tuple(int(n) for n in ROOT.__version__.split("."))
122122

123-
if version >= (6, 44, 0):
123+
if version >= (6, 43, 0):
124124
# Verify that SetHeuristicMemoryPolicy is not available
125125
with self.assertRaises(AttributeError):
126126
ROOT.SetHeuristicMemoryPolicy(True)
127127
ROOT.SetHeuristicMemoryPolicy(False)
128128

129+
if version >= (6, 43, 0) and hasattr(ROOT, "RooArgSet"):
130+
# Verify that the implicit conversion from Python set to RooArgSet
131+
# is gone
132+
x = ROOT.RooRealVar("x", "x", 1.0)
133+
y = ROOT.RooRealVar("y", "y", -1.0)
134+
with self.assertRaises(TypeError):
135+
ROOT.RooDataSet("data", "data", {x, y})
136+
129137
def test_lazy_gdirectory(self):
130138
"""Check that gDirectory is always lazy evaluated to the current
131139
directory.

0 commit comments

Comments
 (0)