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

add python bindings for PIDHandler #397

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

m-fila
Copy link
Member

@m-fila m-fila commented Dec 20, 2024

BEGINRELEASENOTES

  • Added Python bindings for PIDHandler

ENDRELEASENOTES

Python test is selected tests from C++ version
Closes #396

Copy link
Contributor

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

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

Ah nice. I had mainly opened that issue as a reminder for myself not as a call to action ;)

What would be nice to check as well would be the constructors from frames, or from multiple collections via the static from methods. However, they clash with the from keyword from python. That was the main reason for me to not directly go to solve this ;) Would probably need some from_event and from_collections overloads on the c++ side to make things available on the python side in a working manner.

@tmadlener
Copy link
Contributor

Adding the same creation functionality as is available in c++ in python via the from static functions seems not as straight forward as I had hoped. The problem is the python wrapper around the Frame that comes with podio. That makes overload resolution fail, and we would need to introduce a similar wrapper around PIDHandler here to deal with that, I think.

@m-fila m-fila force-pushed the python_pidhandler branch from 5df4f0c to 5ea943a Compare January 24, 2025 16:28
@m-fila
Copy link
Member Author

m-fila commented Jan 24, 2025

There seems to be some problems with a lifetime when using pythonized std::optional and obtaining values from it 😐 For instance these seem to be not always the same

pid1 = handler.getPID(reco[0], 42)
pid1.value()

handler.getPID(reco[0], 42).value()

Possibly related to AIDASoft/podio#642

@m-fila
Copy link
Member Author

m-fila commented Feb 5, 2025

I think a possible workaround for the problems with pythonized std::optional would be to add throwing interface and use it python bindings (and disable optional interface there)

@tmadlener
Copy link
Contributor

If we are adding a dedicated python wrapper / interface here, that could also accommodate the necessary handling of the Frame wrapper.

@m-fila
Copy link
Member Author

m-fila commented Mar 21, 2025

If we are adding a dedicated python wrapper / interface here, that could also accommodate the necessary handling of the Frame wrapper.

I'm not sure if I understand. The functions here are expecting to get a python wrapper for Frame and then get cppyy Frame object from it and use it with the functions expecting C++ Frame

Do you mean you'd like to have reimplementation of C++ functions in python using Frame wrapper methods without reaching to C++ Frame directly?

@tmadlener
Copy link
Contributor

No, I was thinking that if we want the special handling of std::optional for the python PIDHandler, we need to write a python wrapper in any case. But now that I look at the diff again, I see that is already there(?) with the pythonizations doing all the necessary work of reaching through to the cppyy podio::Frame.

So now the actual question is: Can we make the std::optional work "as expected for python" with these pythonizations as well?

@m-fila
Copy link
Member Author

m-fila commented Mar 21, 2025

I see. I tried hiding handling of std::optional inside the pythonizations hoping that if all the checking/unwrapping is done as soon as possible the lifetime issues might not kick-in. But I didn't manage to get it working reliably

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make PIDHandler available and work from python
2 participants