Add muon-catalyzed fusion process/model skeletons#2174
Add muon-catalyzed fusion process/model skeletons#2174sethrj merged 29 commits intoceleritas-project:developfrom
Conversation
Test summary 5 766 files 9 268 suites 17m 33s ⏱️ Results for commit c571e0d. ♻️ This comment has been updated with latest results. |
sethrj
left a comment
There was a problem hiding this comment.
I appreciate the detail classes for the context of long term planning, but I think they should be deleted since they're not used and not tested: having them there is an easy way to introduce zombie code into the codebase.
- Move types to new mucf/Types.hh - Move MucfParticles::from_params to model.cc as a free function - Move all static data to MucfPhysics.cc - Refactor/Remove types in DTMixMucfData - Remove CELER_EXPECT(mu_minus) during MucfProcess construction
Agreed that they are not tested yet, but they are actively being called in the executor. Do you prefer me to make a todo in the executor and remove them until a future PR? |
Oh I didn't catch that. I'll take another look at your comments later this afternoon. |
sethrj
left a comment
There was a problem hiding this comment.
I would prefer to have at least some level of testing (even if it's running through a bunch of null-ops) here. I see now that the executor is being used, but it would be very helpful to have a code coverage tool tell us what is actually being called and tested.
Maybe it's possible to set up a test that creates a state vector and the model (manually setting all the particle attributes that are needed), and calls the step operator for the mucf model?
- Fix import data - Refactor the material calculator to behave as a material inserter - Interactors: define number of secondaries as EnumArrays - Rename internal material id to MucfMatId - Remove find MucfMatId function from DTMixMucfData and make it a lambda in the executor
sethrj
left a comment
There was a problem hiding this comment.
Good enough. I do think we should avoid "skeleton" PRs. I know this is a fairly complex piece of machinery, but in the future let's build up one piece at a time: no skeletons in the code. It's hard to tell if a piece of construction code is correct or optimal if it's not used.
- Fix integral_xs bool - Fwd declare material and particle params - Remove trailing includes
|
@stognini looks like you're missing some |
|
@amandalund @stognini are we ready to merge this? According to codecov, literally none of the new code is tested or even executed; it simply compiles. I really do not like that. It took three weeks to review and fix the builds, and we can't even know if it works yet. Next time we have to put in a big feature, maybe we focus on hardcoding the main use case, then refactoring and refining as we support more fine-grained features. (E.g., start with supporting only DD fusion, one spin, etc., and at least test that it works.) I think right now our muon code has zero coverage at all. |
|
Agree that the lack of testing is a problem. This also is one single executor. E.g. a discrete EM interaction we were in general able to make the interactor and model in one or two PRs. One option, if you'd like, is to revert the ordering I was planning. Keep this as draft, and:
|
|
The interactor we had the ability to test individually, which was great. I don't want to spend any more time on muons than necessary, but I'd say the next follow-on should be a PR with an integration-level test of muon physics: either in |
|
I'm going to merge this out of expedience to reduce the time spent mitigating code rot; @stognini 's next task is to add a unit test that builds the µCF model and compares low level interactor distributions against the G4 code. Then we'll add an integration test that uses muons. |
This PR adds the basic structure for the muon-catalyzed fusion process. See issue #1508 for details.
It is an at rest process, and it applies only to d-t mixtures. The model
executoris responsible for taking a muon at rest and: