Skip to content

[tmva][sofie] Add new operators with feedback from 1st NGT Hackathon #18348

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

sanjibansg
Copy link
Contributor

This PR adds new operators to SOFIE based on the feedback we received from the 1st NGT Hackathon. Following are the missing operators that will be added:

  • CastLike
  • Round
  • Not

Copy link

github-actions bot commented Apr 10, 2025

Test Results

    18 files      18 suites   3d 21h 0m 57s ⏱️
 2 739 tests  2 739 ✅ 0 💤 0 ❌
47 629 runs  47 629 ✅ 0 💤 0 ❌

Results for commit 2d69bc3.

♻️ This comment has been updated with latest results.

@dpiparo
Copy link
Member

dpiparo commented Apr 10, 2025

this is a draft, of course, but a comment about the commit messages: https://github.com/root-project/root/blob/master/CONTRIBUTING.md - please follow the guidelines. I propose to start with "[sofie]" when it's about SOFIE

@sanjibansg sanjibansg marked this pull request as ready for review April 16, 2025 11:19
@sanjibansg sanjibansg requested a review from lmoneta as a code owner April 16, 2025 11:19
Copy link
Member

@lmoneta lmoneta left a comment

Choose a reason for hiding this comment

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

Thanks Sanjiban for the PR.
I see that the NOT operator is implemented as Neg, but it is not exactly the same. The NOT acts only on bool types. Currently Neg supports only Float. We should change this and have support for Boolean in ParseNeg in case it is a NOT

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.

3 participants