-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[tmva][sofie] Disentangle PyMVA and SOFIE #20355
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
base: master
Are you sure you want to change the base?
Conversation
eae03d6 to
8a1bbdc
Compare
8a1bbdc to
2dbbe7d
Compare
2dbbe7d to
3c6791c
Compare
3c6791c to
0305b56
Compare
Test Results 22 files 22 suites 3d 17h 23m 46s ⏱️ For more details on these failures, see this check. Results for commit 5b3842b. ♻️ This comment has been updated with latest results. |
4d736ec to
5b3842b
Compare
lmoneta
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you Jonas for moving the Keras and PyTorch aprser away from PyMVA. A comment I have it is maybe to be more consistent, is to move them not in sofie but in the sofie_parsers.
Something to discuss
Sure, this can be done! I agree it's better to move it into |
So far, the part of SOFIE that uses the C Python API was included in PyMVA for convenience, but the functionality is completely unrelated. Moving this code to SOFIE itself means we can fully build and test SOFIE without enabling `tmva-pymva`. The only subtelty is that we want to be able to disable these SOFIE features that require linking against `libpython`, because this is not always allowed (e.g. in the Python wheels). Therefore, this part is only built if we build also the other ROOT libraries that links against `libpython` besides PyMVA, wich is `TPython`.
This keyword argument is not relevant for defining the model and will cause errors in newer Keras versions.
5b3842b to
faa5deb
Compare
|
Hi @lmoneta, I have implemented your suggestion to moving the parser implementation to I left the tests in |
So far, the part of SOFIE that uses the C Python API was included in PyMVA for convenience, but the functionality is completely unrelated.
Moving this code to SOFIE itself means we can fully build and test SOFIE without enabling
tmva-pymva.The only subtelty is that we want to be able to disable these SOFIE features that require linking against
libpython, because this is not always allowed (e.g. in the Python wheels). Therefore, this part is only built if we build also the other ROOT libraries that links againstlibpythonbesides PyMVA, wich isTPython.