Add test_bnd_prt_inflow with nascent Inflow class#343
Add test_bnd_prt_inflow with nascent Inflow class#343JamesMcClung merged 29 commits intopsc-code:mainfrom
Inflow class#343Conversation
germasch
left a comment
germasch
left a comment
There was a problem hiding this comment.
Looks good to me! (other than some minor nitpicks)
The one thing I don't love is the repetitiveness of the tests. But I'm not sure factoring out some common pieces won't make readability worse...
| if (patch.off[inj_dim_idx_] != 0) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
There is a at_boundary_lo() or something of that sort somewhere, but not sure it's easily reusable here.
There was a problem hiding this comment.
I see mrc_domain_at_boundary_lo, but yeah, doesn't seem usable here
| std::vector<psc::particle::Inject> prts; | ||
| }; | ||
|
|
||
| double half() { return 0.5; } |
There was a problem hiding this comment.
Minor nitpick: I'd prefer this to be static, though since it's just in a test source, I guess it doesn't really matter.
There was a problem hiding this comment.
Definitely inj_dim_idx_ should have been static (and it now is), but did you mean also half()? It is too now.
There was a problem hiding this comment.
I'm less convinced that inj_dim_idx_ should be static, I just meant the half().
There was a problem hiding this comment.
Okay, so looking at your changes, what in theory sounds better would be would be to template the whole class by INJ_DIM_IDX in the first place -- I don't think it makes sense to have it instantiated with, e.g., Dim_xy currently.
That'd cause some hassle, since the AdvanceParticle does need a corresponding Dim_... argument. And it would actually even make sense to potentially have it take both template args, ie, 1 a.k.a. y and Dim_yz for the kind of simulation you actually currently run. So I guess I'd say, if something has a DIM template arg, that should always be the dimensionality of the underlying simulation -- it shouldn't be abused the way it's been done here (my bad...)
There's probably an argument to try to avoid widespread use of a hard-coded (templated) Dim in the first place, but that's for another day to think through...
There was a problem hiding this comment.
If the injection dimension is y, and we care about pushing along the x and/or z dimensions as well, wouldn't that necessitate another check at each domain corner to ensure particles don't leave the x or z bounds? Or is it potentially even worse, because even when in bounds, they might end up getting injected into the wrong patch?
That currently can't happen (provided only a 1d dim_... is passed). Physically, I suppose it makes sense to support that for cases where the injected particle velocity distributions vary along the boundary, but that isn't the case for now at least. I also wouldn't want there to be some subtle pitfall for future users who might need such a feature, though. I'm personally inclined to decouple the AdvanceParticle dim and inject_dim_idx, and throw in an assert that they match with a comment explaining the problem.
More template args are definitely annoying, but maybe the performance benefits outweigh the ergonomic downsides; I don't know, and I'm happy to defer to your opinion on that.
and make it a function pointer. Lambdas would require templating the type (and thus obfuscate the code), and avoid std::function's virtual function calls, since this could be called many times per step
the removed ones were copied and unchanged
JamesMcClung
force-pushed
the
bnd-prt-inflow
branch
from
38dc75c to
05e2f55
Compare
2 participants
Eventually,
Inflowwill be moved and used by open particle boundary conditions. Its current state is mostly usable, but could be optimized by removingstd::functioncalls.