Skip to content

Conversation

@siliataider
Copy link
Contributor

This failure appeared when testing the python wheels in #19600:

    legend2.SetFillColor("kWhite")
    ~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^
TypeError: none of the 2 overloaded methods succeeded. Full details:
  void TAttFill::SetFillColor(TColorNumber) =>
    TypeError: could not convert argument 1
  short conversion expects an integer object

@siliataider siliataider requested a review from couet as a code owner October 31, 2025 14:38
@siliataider siliataider added the skip ci Skip the full builds on the actions runners label Oct 31, 2025
@siliataider siliataider self-assigned this Oct 31, 2025
@vepadulano
Copy link
Member

@guitargeek I vaguely remember some Pythonizations in RooFit that converted strings with the same name as ROOT constants to the actual constant value to allow for situations similar to the one of this tutorial. Maybe this particular overload is not Pythonized? I could also be misremembering.

Copy link
Member

@vepadulano vepadulano left a comment

Choose a reason for hiding this comment

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

Thanks a lot! While we understand the situation of the Pythonizations better I leave two very minor comments.

Copy link
Contributor

@guitargeek guitargeek left a comment

Choose a reason for hiding this comment

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

Thanks a lot! I'd just suggest so refactor the tutorial such that we don't rely on Pythons variable scoping.

@guitargeek
Copy link
Contributor

guitargeek commented Nov 3, 2025

@guitargeek I vaguely remember some Pythonizations in RooFit that converted strings with the same name as ROOT constants to the actual constant value to allow for situations similar to the one of this tutorial. Maybe this particular overload is not Pythonized? I could also be misremembering.

This looks like a bug in the implicit conversion of cppyy. It should work, because TColorNumber can be constructed from a string:
https://github.com/root-project/root/blob/master/core/base/inc/TColor.h#L145

So we can merge this PR to make things work, but also open a GitHub issue about this and revert the change once the string version works again (probably with the cppyy update by @Vipul-Cariappa and @aaronj0). I will investigate and open such an issue.

@guitargeek
Copy link
Contributor

guitargeek commented Nov 3, 2025

Ok, it has nothing to do with cppyy, I just didn't know that two implicit conversions at once are not allowed in C++ 🙂

So this can be fixed on the C++ side: #20271

Let's see how the CI responds to that PR, but if it works In don't think we need to change from "kWhite" back to the more verbose ROOT.kWhite. It would still be worth it to do the refactoring to avoid relying on Python-specific variable scoping though!

Thanks a lot for catching this in any case

@siliataider
Copy link
Contributor Author

Sure, then I'll wait for the status of that PR before changing this one

Copy link
Contributor

@guitargeek guitargeek left a comment

Choose a reason for hiding this comment

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

Thank you very much!

@siliataider siliataider force-pushed the docs branch 3 times, most recently from 81233b5 to ea85f97 Compare November 4, 2025 16:37
@siliataider siliataider changed the title [skip-ci][tutorials] fix legend style in roofit tutorial [skip-ci][tutorials] refactor roofit tutorial Nov 4, 2025
@siliataider siliataider merged commit 264da1e into root-project:master Nov 6, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

in:Documentation skip ci Skip the full builds on the actions runners

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants