-
Notifications
You must be signed in to change notification settings - Fork 45
Add options for constructing an ORANGE geometry #2045
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: develop
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2045 +/- ##
===========================================
- Coverage 85.53% 82.85% -2.68%
===========================================
Files 1696 1618 -78
Lines 75824 65367 -10457
Branches 4174 3859 -315
===========================================
- Hits 64853 54163 -10690
+ Misses 9466 9463 -3
- Partials 1505 1741 +236 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Test summary 4 946 files 7 871 suites 5m 31s ⏱️ Results for commit 0ddc337. ♻️ This comment has been updated with latest results. |
55c1b85 to
0ddc337
Compare
|
@sethrj can you take a preliminary look at this? We already have |
| struct ConstructionOptions | ||
| { | ||
| //! Options for Bounding Interval Hierarchy (BIH) construction | ||
| detail::BIHBuilder::Options bih_options; |
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.
It might make it more messy in the short term, but I think the preferred pattern will look more like the newer celeritas code: inp/Foo.hh has the setup options for Foo. So we'd have orange/inp/Bih.hh which would have struct inp::Bih and that would replace BihBuilder::Options. We'd likewise move orange/gorg/Options.hh to orange/inp/Converter.hh. Finally we'd also move OrangeInput to orange/inp/Model.hh or similar.
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.
FWIW, I think Options makes more sense, lest we overload the word "input"; we already have an orangeinp folder. Likewise, below, I think semantically the input to the BIHBuilder are the bounding boxes, whereas the min_split_size is an option specifying how the process is done.
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.
The orange/inp folder doesn't currently exist. I can make, but I am still not clear on how InputBuilder::Options fits in (seems like that should be defined at the OrangeInput level).
I also, I think g4org::Options should consist of: 1) a high-level struct will all general OrangeOptions, that is also used for a non-G4 workflow to specify all ORANGE options in one place, and 2) additional G4-specific options.
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.
The weirdness/confusion is because we've been inconsistent in the past and are transitioning toward having an inp namespace that has all of the input structs that were formerly called Input/Options. This namespace is to be the primary interface with the outside parts of the code: see #2029.
In a hypothetical/future refactorization that I am talking about:
celeritas::OrangeInput->celeritas::inp::Orangeceleritas::detail::BIHBuilder::Options->celeritas::inp::BihBuilderceleritas::g4org::Options->celeritas::inp::Converter(?)celeritas::orangeinp->celeritas::setup(?)celeritas::g4org->celeritas::setup::g4org(?)
I don't know what's the right path between that future and our present, but I would at least like:
- BIH builder doesn't include half of orange transitively via the OrangeInput (since VariantSurfaces)
- OrangeInput doesn't include and rely on a detail namespace
- Mlutiple layers of nested options with only one member per struct
| //! Input parameters | ||
| struct Input | ||
| //! Options parameters | ||
| struct Options |
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.
if we move the struct, this'll become using Input = inp::BihBuilder; as is done in the other update celeritas classes.
This MR adds a
ConstructionOptionsstruct in order to facilitate the study of performance-sensitive parameters that govern ORANGE geometry construction.