Use infix evaluation

[esseivaju]

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.

[github-actions[bot]]

Test summary

 5 673 files   9 129 suites   18m 41s :stopwatch:  2 077 tests  2 051 :white_check_mark:  26 :zzz: 0 :x: 31 827 runs  31 679 :white_check_mark: 148 :zzz: 0 :x:

Results for commit 67e95e92.

:recycle: This comment has been updated with latest results.

[esseivaju]

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

[sethrj]

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]

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]

@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 🤔

[elliottbiondo]

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

[sethrj]

@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 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 🤔

Most of the .org.json files don't have a matching GDML, are they available somewhere? Not sure what's the best way to regenerate all these data files

[codecov[bot]]

Codecov Report

:x: Patch coverage is 80.34682% with 34 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 84.90%. Comparing base (9b9424a) to head (e3f08ec). :warning: Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
src/orange/detail/LogicUtils.cc	75.73%	32 Missing and 1 partial :warning:
src/orange/OrangeInputIO.json.cc	88.88%	0 Missing and 1 partial :warning:

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1565      +/-   ##
===========================================
- Coverage    84.92%   84.90%   -0.02%     
===========================================
  Files         1245     1245              
  Lines        43764    43925     +161     
  Branches     16502    16564      +62     
===========================================
+ Hits         37166    37295     +129     
- Misses        4828     4858      +30     
- Partials      1770     1772       +2     

Files with missing lines	Coverage Δ
src/orange/OrangeData.hh	91.12% <100.00%> (+0.16%)	:arrow_up:
src/orange/OrangeInput.hh	82.60% <100.00%> (+2.60%)	:arrow_up:
src/orange/OrangeParams.cc	83.33% <100.00%> (+0.24%)	:arrow_up:
src/orange/OrangeParamsOutput.cc	100.00% <100.00%> (ø)
src/orange/OrangeTypes.cc	81.81% <100.00%> (+0.64%)	:arrow_up:
src/orange/OrangeTypes.hh	88.23% <ø> (ø)
src/orange/OrangeTypesIO.json.cc	100.00% <100.00%> (ø)
src/orange/detail/LogicUtils.hh	100.00% <ø> (ø)
src/orange/orangeinp/UnitProto.cc	93.02% <100.00%> (+0.02%)	:arrow_up:
src/orange/orangeinp/UnitProto.hh	92.00% <100.00%> (+0.33%)	:arrow_up:
... and 4 more

... and 5 files with indirect coverage changes

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[esseivaju]

@sethrj That should cover 1-3 from your previous comment

[esseivaju]

convert_to_postfix

What would be the use case for this since we always go from postfix->infix?

[sethrj]

convert_to_postfix

What would be the use case for this since we always go from postfix->infix?

Sorry didn't see this earlier. The first use case would be to avoid rewriting all the unit tests and reference logic; the second more important case is that we may be integrating with the legacy ORANGE tracking code in SCALE, which requires infix notation.

[esseivaju]

We have a one-liner to toggle infix/postfix evaluation now but there are many broken tests with infix that'll need fixed...

[sethrj]

We have a one-liner to toggle infix/postfix evaluation now but there are many broken tests with infix that'll need fixed...

To make life easier, couldn't we just apply the "infix-to-postfix" conversion at that point? Or would the application of De Morgan's law still mean we'd get different results? Perhaps we should as you suggest always require "remove negated joins"; we could do that (plus any lingering massive changes) as part of a separate PR before flipping the result.

[esseivaju]

To make life easier, couldn't we just apply the "infix-to-postfix" conversion at that point? Or would the application of De Morgan's law still mean we'd get different results? Perhaps we should as you suggest always require "remove negated joins"; we could do that (plus any lingering massive changes) as part of a separate PR before flipping the result.

I'd expect results to be identical modulo any bugs. At this point I don't think we should require the simplification until we know how expensive it is to do during initialization and we've caught any potential bugs with more realistic test geometry than the unit tests we have.

We can follow-up in separate PR, closing this one before the year mark 😅

[sethrj]

Sounds great. Thanks @esseivaju for getting this through!