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 auxiliary extensible params and state #1278

Merged
merged 13 commits into from
Jun 18, 2024

Conversation

sethrj
Copy link
Member

@sethrj sethrj commented Jun 17, 2024

For #886 it would be good to start storing state data alongside the core state rather than using the hacky stream store to make the params class mutable.

  • Params classes (e.g. actions) can inherit from UserParamsInterface
  • UserParamsInterface has a function for the params to create a state (it can be called multiple times with a memspace, stream ID, and size)
  • A UserParamsRegistry holds an array of user params, each associated with a UserId
  • A UserStateVec is created alongside the core state and it holds the stream-local state corresponding to each user params
  • A UserStateData helper class can wrap a FooStateData (collection group)
  • A get helper function for getting states from a user

A follow-up PR refactors the optical data using this new change set.

Closes #392 .


Non-critical follow-on work: how to combine and output params data with the results of multiple states. Currently the states are unique pointers owned by the core state and created when we build the stepper, and outputs are usually shared pointers that are registered when the params are created. Maybe we should punt on the output for now (so we can get this into the optical state) and consider having a separate "multithread output" class, or using actions to integrate over the streams at the end of a run, or something...

The action diagnostic execution kernel looks like this:

void ActionDiagnostic::execute(CoreParams const& params,
                               CoreStateHost& state) const
{
    auto execute = make_active_track_executor(
        params.ptr<MemSpace::native>(),
        state.ptr(),
        detail::ActionDiagnosticExecutor{
            this->ref<MemSpace::native>(),
            state.user_state_data<ParticleTallyStateData>(user_id_)});
    return launch_action(*this, params, state, execute);
}

@sethrj sethrj added enhancement New feature or request core Software engineering infrastructure labels Jun 17, 2024
@sethrj sethrj requested review from amandalund and esseivaju June 17, 2024 14:35
@sethrj sethrj force-pushed the core-data-storage branch 4 times, most recently from e8200a5 to 918da58 Compare June 17, 2024 19:37
@sethrj sethrj force-pushed the core-data-storage branch from 918da58 to 56e237f Compare June 17, 2024 19:38
@sethrj sethrj marked this pull request as ready for review June 17, 2024 19:39
Copy link
Contributor

@esseivaju esseivaju left a comment

Choose a reason for hiding this comment

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

Nice! LGTM, just a few minor comments.

src/celeritas/global/CoreState.cc Show resolved Hide resolved
{
//---------------------------------------------------------------------------//
//! Default destructor.
UserStateInterface::~UserStateInterface() = default;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, is the out-of-line definition needed?

src/corecel/data/UserParamsRegistry.hh Outdated Show resolved Hide resolved
src/corecel/data/UserStateVec.hh Outdated Show resolved Hide resolved
Copy link
Contributor

@amandalund amandalund left a comment

Choose a reason for hiding this comment

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

This looks great, thanks @sethrj! The only thing I'm a little unsure about is the name User; it definitely fits for the diagnostics, but maybe not as much when we're using this for the optical data.

@sethrj sethrj enabled auto-merge (squash) June 18, 2024 14:29
@sethrj sethrj changed the title Add user-extensible params and state Add auxiliary extensible params and state Jun 18, 2024
@sethrj sethrj merged commit 17d96db into celeritas-project:develop Jun 18, 2024
29 checks passed
Copy link
Contributor

@amandalund amandalund left a comment

Choose a reason for hiding this comment

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

A couple Users left (in case you want to replace them in #1280).

@@ -54,6 +55,7 @@ class CoreParams final : public ParamsDataInterface<CoreParamsData>
using SPConstWentzelOKVI = std::shared_ptr<WentzelOKVIParams const>;
using SPActionRegistry = std::shared_ptr<ActionRegistry>;
using SPOutputRegistry = std::shared_ptr<OutputRegistry>;
using SPUserRegistry = std::shared_ptr<AuxParamsRegistry>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
using SPUserRegistry = std::shared_ptr<AuxParamsRegistry>;
using SPAuxRegistry = std::shared_ptr<AuxParamsRegistry>;

@@ -73,6 +74,7 @@ class GlobalTestBase : public Test

using SPActionRegistry = SP<ActionRegistry>;
using SPOutputRegistry = SP<OutputRegistry>;
using SPUserRegistry = SP<AuxParamsRegistry>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
using SPUserRegistry = SP<AuxParamsRegistry>;
using SPAuxRegistry = SP<AuxParamsRegistry>;

@sethrj sethrj deleted the core-data-storage branch July 23, 2024 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Software engineering infrastructure enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Store extra state data on state rather than actions
3 participants