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

Implement discrete optical physics #1604

Open
wants to merge 55 commits into
base: develop
Choose a base branch
from

Conversation

hhollenb
Copy link
Contributor

Implementation of PhysicsData and associated views necessary to do volumetric optical physics.

PhysicsParams closely matches the core physics version, but slimmed down just for discrete interactions. The DiscreteSelectAction, PhysicsTrackView, and PhysicsStepView were implemented similarly. I've updated the hooks into the core optical classes so executors should have access to physics data.

Currently only absorption and rayleigh models are implemented, and I've limited the importing of models in tests to just those two.

I've left the executor implementation empty for the moment to reduce the overall complexity of the merge. I can start working on them and adding them to this PR. I can also add some extra device tests and I/O tests for optical data.

Hollenbeck-Hayden and others added 30 commits November 8, 2024 00:55
@hhollenb hhollenb added enhancement New feature or request physics Particles, processes, and stepping algorithms labels Jan 30, 2025
Copy link

github-actions bot commented Jan 30, 2025

Test summary

 4 197 files   6 452 suites   13m 36s ⏱️
 1 754 tests  1 741 ✅ 11 💤 2 ❌
22 571 runs  22 484 ✅ 83 💤 4 ❌

For more details on these failures, see this check.

Results for commit b27891b.

♻️ This comment has been updated with latest results.

Copy link
Member

@sethrj sethrj left a comment

Choose a reason for hiding this comment

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

Great work and very nice unit test! Sorry it took me so long to dive into it...

return num_models > 0 && model_to_action >= 1;
}

//! Indicate a discrete interaction was rejected by integral method
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//! Indicate a discrete interaction was rejected by integral method
//! Undergo a discrete interaction

/*!
* Access step-local (non-persistent) optical physics track data.
*/
class PhysicsStepView
Copy link
Member

Choose a reason for hiding this comment

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

Since optical physics is so much simpler than full EM, I'm not sure we need a separate "step" view. Let's discuss at today's meeting?


//---------------------------------------------------------------------------//
/*!
* Physics state data for a single track.
Copy link
Member

Choose a reason for hiding this comment

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

Nowadays we're preferring SOA rather than AOS, and I think it would be better not to blend the "persistent" (across steps) with the "temporary" (within a step) in the same data structure: can you replace this struct with two StateItems<real_type?.

using ValueGrid = GenericGridRecord;
using ValueGridId = OpaqueId<ValueGrid>;
using ValueTable = ItemRange<ValueGrid>;
using ValueTableId = OpaqueId<ValueTable>;
Copy link
Member

Choose a reason for hiding this comment

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

We should sync up with @amandalund about these. The requirements for optical will be much simpler (we'll just be using nonuniform grids I think?), so I don't think we'll even need them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, and yeah AFAIK only nonuniform grids.

namespace optical
{
//---------------------------------------------------------------------------//
/*!
Copy link
Member

Choose a reason for hiding this comment

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

Need to add a line of documentation here and elsewhere...

//---------------------------------------------------------------------------//
/*!
*/
struct AbsorptionExecutor
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should move these to a subsequent PR?

{
return std::make_shared<MockModel>(id);
}
};
Copy link
Member

Choose a reason for hiding this comment

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

It would make the file a bit easier to read if you move these classes to a separate file.

protected:
using RngEngine = ::celeritas::test::DiagnosticRngEngine<std::mt19937>;

ModelId::size_type const num_models = 4;
Copy link
Member

Choose a reason for hiding this comment

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

maybe static constexpr?

//// Data ////

StateItems<PhysicsTrackState> states;
Items<real_type> per_model_xs; //!< XS [track][model]
Copy link
Contributor

Choose a reason for hiding this comment

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

Since there are only ever two or three models (processes) and the track doesn't lose energy along the step, we may want to consider the tradeoff of storing this extra state data vs. recalculating the cross section on the fly in select_discrete_interaction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request physics Particles, processes, and stepping algorithms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants