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

Use infix evaluation #1565

Draft
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

esseivaju
Copy link
Contributor

@esseivaju esseivaju commented Jan 7, 2025

Use infix evaluation of logic expressions. We probably want to give a compile-time or run-time option to pick between infix and postfix notation.

@esseivaju esseivaju added enhancement New feature or request orange Work on ORANGE geometry engine labels Jan 7, 2025
Copy link

github-actions bot commented Jan 7, 2025

Test summary

  432 files    679 suites   31s ⏱️
1 354 tests 1 200 ✅ 40 💤 114 ❌
2 467 runs  2 240 ✅  8 💤 219 ❌

For more details on these failures, see this check.

Results for commit a8542c5.

♻️ This comment has been updated with latest results.

@esseivaju
Copy link
Contributor Author

@sethrj @elliottbiondo Is this failing because we need to update all the orange test data which is in postfix notation, e.g. here

@sethrj
Copy link
Member

sethrj commented Jan 7, 2025

Oh boy. Let's chat about this on slack? Namely if we're going to hardcode an option, whether and where to test it... And if we're going to break all the CSG representation strings, one thing I would like to do is flip the values of Sense so that "inside sphere 0" gives a CSG representation of "0" rather than "~0". 🤔

@esseivaju
Copy link
Contributor Author

Yep, we can discuss this on Slack. I wanted to get numbers from the regression suite, so this is a quick, hardcoded implementation, but there are also logic strings in the regression suite.

@sethrj
Copy link
Member

sethrj commented Jan 7, 2025

@esseivaju I think you can rebuild those manually by calling

$ export GDML=foo.gdml
$ build/test/orange/g4org_Converter '--gtest_filter=*arbitrary' --gtest_also_run_disabled_tests

and that'll spit out the necessary JSON files locally.

That might not be true for the custom TestEM3 🤔

@sethrj sethrj linked an issue Jan 8, 2025 that may be closed by this pull request
@elliottbiondo
Copy link
Contributor

If infix/postfix could be toggled as a runtime option that would side-step the issue with the orange test data, correct?

@sethrj
Copy link
Member

sethrj commented Jan 8, 2025

@elliottbiondo yeah, but since it requires different implementations deep in SimpleUnitTracker that would mean propagating a lot of templates... I think we'd want this switch to be all or nothing.

@esseivaju esseivaju force-pushed the use-infix-evaluation branch from c0ef7ce to 05f8541 Compare January 10, 2025 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request orange Work on ORANGE geometry engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement short-circuit volume evaluation using infix notation
4 participants