[Python] Deprecate conversion from Python set to RooArgSet#22657
Conversation
guitargeek
force-pushed
the
rooargset
branch
2 times, most recently
from
602fa89 to
1ef2f17
Compare
guitargeek
changed the title
Test Results 22 files 22 suites 3d 13h 11m 43s ⏱️ Results for commit b2c978c. ♻️ This comment has been updated with latest results. |
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 root-project#8751) whithout thinking
too much about this footgun.
This pattern should not be used, because it leaves the RooArgSet in a non-deterministic ordering, depending on the implementation of Python.
lmoneta
left a comment
lmoneta
left a comment
There was a problem hiding this comment.
LGTM! I agree to deprecate these conversions.
Are the changes breaking the current Python code using RooFit? This could be a potential issue, it should probably be very well documented!
I don't think it doesn't break much Python code using RooFit in practice, because this implicit conversion was only introduced 5 years ago and is mainly used by the RooFit tutorials. Not sure exactly how much users picked it up, but I have not seen it in user code so far. The release notes are including a clear statement about this documentation. |
2 participants
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:
Gives on my system:
This is not even deterministic, because Python doesn't guarantee that
a
setis ordered in any way.I introduced this 5 years ago in commit guitargeek@c54c324
(PR #8751) without thinking
too much about this footgun.