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 a hack to override ORANGE max intersections/faces #1430

Merged
merged 10 commits into from
Oct 1, 2024

Conversation

sethrj
Copy link
Member

@sethrj sethrj commented Sep 27, 2024

See #1334 . This should allow us to restore the performance of TestEM3 temporarily, although we should disclose that this hack is being used when we present our test problems.

This will only help if all the visited volumes have fewer faces/intercepts than the given value: otherwise it will crash.

I'm going to test this before merging.

@sethrj sethrj added minor Minor internal changes or fixes orange Work on ORANGE geometry engine performance Changes for performance optimization labels Sep 27, 2024
Copy link

github-actions bot commented Sep 27, 2024

Test summary

 3 264 files   5 062 suites   3m 42s ⏱️
 1 524 tests  1 497 ✅ 27 💤 0 ❌
16 884 runs  16 821 ✅ 63 💤 0 ❌

Results for commit c43ff2f.

♻️ This comment has been updated with latest results.

<< mfi_hack_envname << "='" << mfi << "'";

ForceMax result;
static std::regex const mfi_regex{R"re(^(\d+),(\d+)$)re"};
Copy link
Contributor

Choose a reason for hiding this comment

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

In the context of this being a presumably temporary hack I don't know if this matters, but it the regex validation is complex enough to get its own function in anonymous namespace.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah this option shouldn't live long (the "cheat" itself isn't tested anywhere except by me manually for our regression problems). The regex ^(\d+),(\d+)$ isn't that complicated (whole text, capture two nonnegative integers separated by a comma) but perhaps it's harder to read because of the literal R"re()re" string.

@sethrj sethrj marked this pull request as ready for review October 1, 2024 15:04
@sethrj
Copy link
Member Author

sethrj commented Oct 1, 2024

OK, using the "flat" file changes the results because of MSC:
rel-work

But the throughput for the "identical geometry in memory" goes up:
rel-throughput

@sethrj sethrj enabled auto-merge (squash) October 1, 2024 19:13
@sethrj sethrj merged commit 5b8aa5e into celeritas-project:develop Oct 1, 2024
32 checks passed
@sethrj sethrj deleted the orange-hack branch October 1, 2024 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Minor internal changes or fixes orange Work on ORANGE geometry engine performance Changes for performance optimization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants