Polymorphic injectors, external currents, and diagnostics by JamesMcClung · Pull Request #377 · psc-code/psc

JamesMcClung · 2026-02-03T14:43:15Z

This is the first of (hopefully) several PRs aiming to simplify the process of making a PSC integrator, while also making it more flexible.

As the title says, this PR makes injectors, external currents, and diagnostics inherit from a base class, and the PSC integrator has lists of pointers to each of those base classes.

Benefits of this approach:

Remove the need for amalgamative types like CompositeInjector and DiagnosticsDefault
Remove the need for empty types like ExternalCurrentsNone and InjectParticlesNone
Opting out of e.g. diagnostics requires 0 code, simplifying tests
No need to lock-in specific types for these objects in PscConfig types

Downsides of this approach:

Virtual functions can't be templated, so the classes themselves must be templated by the function parameters (MfieldsState and Mparticles). This would be a problem if these objects a) needed to accept different types of arguments, and b) were expensive to create. Neither (a) nor (b) is currently the case.
Virtual functions can't be inlined (and they require a vtable lookup, but that's relatively negligible), so this approach shouldn't be applied in hot spots.

It seems to me that every component of the PSC integrator could be polymorphic instead of templated, and that is one of the long-term goals of this PR chain.

germasch

Overall, I definitely like the clean up that comes by adding things after the constructor, and specifically having a list of external currents / injectors, etc, that makes it cleaner (including the default case of nothing) and more powerful.

I do have one little issue with storing the raw pointers / leaking the objects (not that the leaking as such matters, but I don't think it's too hard to prevent that).

What I'm somewhat less sure of is whether there's really much gained by having polymorphism here, in particular since essentially all the base type does is specify a given call signature, which could be done via std::function<> directly (if one were to rename the methods back to operator(), which I think I'd still prefer). I don't know, I haven't fully thought this through, in particular questions of ownership / context / copying (or avoiding them), so that's something we can discuss later, after this is merged.

src/include/injector_base.hxx

germasch · 2026-02-09T17:31:21Z

src/include/boundary_injector.hxx

+  void inject(Mparticles& mprts, MfieldsState& mflds) override
+  {
+    (*this)(mprts, mflds);
+  }
+


I may learn more as I go through more commits, but at this point I'm wondering, why not stick with operator()?

I would say explicit method names helps with discoverability: one can search for implementations of named methods, but not for operator(). That's also a benefit of using polymorphism rather than templates or outright type erasure (à la std::function).

Following my own logic, I should get rid of the lambda wrappers and force users to make their own subclasses. That would be better future-proofed against more methods, too (such as requiring subclasses to provide logging-related information).

I guess I'm not entirely clear on my own thinking here, though I think it's nice isolating users from having to do their own subclasses -- though admittedly lambda functions are likely an even more unfamiliar feature to a casual user...

Anyway, I guess what's there currently (class from lambda) is fine for the time being.

germasch · 2026-02-09T17:33:22Z

src/include/psc.hxx

+  std::vector<InjectorBase<Mparticles, MfieldsState>*> injectors;
+


I think I read somewhere that ideally modern C++ shouldn't use explicit pointers at all. The question here is, what guarantees that the objects pointed to don't go away?

I kinda felt like using fancy pointer wrappers would be more performative than actually beneficial, since the psc integrator may as well be a singleton, and an entire case revolves around calling integrate() once. Thus the leaking :^)
But I'll go back and do the right thing...

Actually, what would be the best option, if not unique_ptr?

As I'm working on switching to unique_ptrs, I'm finding it annoying to have to dynamically allocate all the components. I'm leaning back towards keeping raw pointers, since it's nicer to just stack-allocate things.

I guess I haven't looked at the various places where this is being used, the ones I saw initially had a new anyway. I think transferring ownership is cleaner. I suppose one could have add_injector() or whatever dynamically copy construct the thing, which would hide the new, but at the expense of needing the injectors to be copy-constructible.

The other alternative I can think of is to just keep a reference, ie., punt on the ownership question -- the one thing that's nice about that is that it guarantees non-null, but generally speaking, I think clear ownership rules are preferable. And I can kinda think of test cases where one might want to have to integrators going at the same time for some kind of cross checking, where it'd generally be nice to not assume singleton behavior, though it's kinda far fetched.

The news were there to avoid having to stack-allocate on a separate line, which references can't do. I realize that isn't a good justification for raw pointers. I'll try to find a better solution that achieves clear ownership without introducing excessive boilerplate.

germasch · 2026-02-09T17:37:29Z

src/libpsc/tests/test_boundary_injector.cxx

+  psc.injectors.push_back(
+    new BoundaryInjector<ParticleGenerator, typename PscConfig::PushParticles>(
+      ParticleGenerator(1, 1), grid));


I guess this answers the question about the injectors not going away, essentially because they're being leaked ;) Obviously not a big issue as such, but I think unique_ptr may be the better option here.

Another thing I don't fully like is that the injectors are added after the fact, which kinda means that the integrator isn't fully set up after being constructed. But I think it's acceptable here, in that an empty list of integrators is a valid state.

And I would have to agree that, as we currently do, passing N objects to the constructor makes that rather ugly...

The partially-constructed state of the integrator makes me uncomfortable, too... and I was only planning to make it worse over the next few weeks. This seems like a perfect place to use the builder pattern, but it might be a while before that end state is reached.

src/include/psc.hxx

JamesMcClung · 2026-02-10T14:17:41Z

Some other things I'd like to build into these components are:

A name, used for logging.
Parameters like interval and a public should_run(int timestep) method

germasch

👍

JamesMcClung added 22 commits February 3, 2026 09:12

+injector_base: InjectorBase, InjectorFromLambda

f62ec7c

boundary_injector: inherit from InjectorBase

496ad5c

psc: have a list of InjectorBase ptrs

7a35b95

*: add injectors to the list

a175f13

psc; *: rm templated injector

99beaf1

psc: -InjectParticlesNone

1be2d52

-composite_injector; *

1c2ddfb

+external_current_base

3931fad

psc; *: use vec of ExternalCurrents

0e6f2a5

psc: -ExternalCurrentNone

ddc197a

psc; *: make vecs private

bc43fb1

boundary_injector: don't impl op()

358a042

psc; *: initialize -> pre_first_step

c3fbc68

+diagnostic_base

f4a58a6

OutputFieldsDefault: make diagnostic

7ee609c

output_particles*; *: make diagnostic

dceac22

DiagEnergies; *: make diagnostic

ad80822

psc; *: abstract diagnostics

95256de

psc: update stat in perform_diagnostics

77fa1a5

-DiagnosticsDefault; *

119a734

tests: don't make diagnostics

17fe7ab

psc: fix capture by ref -> segfault

70c54da

germasch requested changes Feb 9, 2026

View reviewed changes

injector_base: lambda_

bb1298b

JamesMcClung added 3 commits February 10, 2026 10:07

*_base: more lambda_

dc5cf86

psc: assert pointers

3c16414

_*base: include functional

79e444a

germasch approved these changes Feb 14, 2026

View reviewed changes

		std::vector<InjectorBase<Mparticles, MfieldsState>*> injectors;

Conversation

JamesMcClung commented Feb 3, 2026

Uh oh!

germasch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JamesMcClung commented Feb 10, 2026

Uh oh!

germasch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants