-
Notifications
You must be signed in to change notification settings - Fork 225
Fusion Extension implementation - inter-species #5479
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
base: dev
Are you sure you want to change the base?
Fusion Extension implementation - inter-species #5479
Conversation
de424c1 to
0030916
Compare
0030916 to
91b1b3d
Compare
|
Right now this reuses a lot of collision code simply by copying files and also somewhat modifying them. We should unify this, for example, by generalizing the collision kernels. I'm not sure if this should happen before this is merged. @psychocoderHPC what do you think? |
91b1b3d to
6e79650
Compare
we should keep the code first seperate, after merging we have a baseline to validate if we break something. Than we should check what we can unify. |
|
@FilipO28555 the CI is still complaining that some source code files are marked as executable, for example,
only binaries and scripts should be marked as executable, please follow the directions in the CI-job output on how to fix this. |
b23ee4d to
25f1789
Compare
PrometheusPi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FilipO28555 This is not yet a full review, but it would be great if you remove unused /comment-out code so that the number of added files gets less scary (right now it's 3.5k lines)
include/picongpu/param/fusion.param
Outdated
| * parameterization from experimental data. | ||
| * Source: https://hdl.handle.net/11858/00-001M-0000-0027-6535-1 (page 29) | ||
| */ | ||
| // struct DT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove uncommented /unused code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One option if you want to keep this code is to add it as an example and activate it. That way we get better CI testing as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, Now the fusion.param is a bare minimum implementation - I moved the examples to documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you have really moved all the example code into the documentation.
We avoid doing this because it is easy to overlook updating the docs when something in your implementation changes, and we end up with code in our docs which doesn't compile.
The way we deal with this is to put the param file into an example/test set up under share/picongpu/examples or share/picongpu/tests and then include this code into the doxygen. This way the code is checked for compilation and if something in the code changes, you don't have to separately updated the docs.
You can look at this to see how to include code into doxygen.
|
Also I'm not sure how/if possible it is anymore but it would have been useful to have a commit at the state where you simply copied code from the collision pipeline and made it compile, and then added the fusion changes on top with separate commits. |
a63cfc4 to
1358f3d
Compare
Features: - binary fusion reactions between two different species - choosing product weights depending on particles masses and charge. - documentation
3de3379 to
d3ad252
Compare
- Remove all verbose comments and examples from fusion.param - Move complete D-T fusion example to documentation
e1e5543 to
4dc43c7
Compare
|
@psychocoderHPC any progress on reviewing this PR? |
psychocoderHPC
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please review your own changes a remove hard coded numbers e.g. 1.0 by the correct precision of the value e.g. 1.0_COLL to avoid silent implicit casts.
include/picongpu/param/fusion.param
Outdated
|
|
||
| constexpr uint32_t cellListChunkSize = TYPICAL_PARTICLES_PER_CELL; | ||
| constexpr float_X productMinWeighting = 16.1; | ||
| constexpr uint32_t maxFmult = 1e6; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add a doc string and unit, additionally the type is an integral type but you assigned an double value.
88cd43f to
636b378
Compare
636b378 to
886fde7
Compare
|
@psychocoderHPC a few CI jobs failed with due to script errors, please restart them |
|
CI job restarted |
|
I triggered the CI again, we have some connectivity issues:
|
This extension is based on papers:
Wu et al
PIC code Chicago
Cannoni
Cross section parametrization:
Bosh, Hale
There is now documentation.
Code flow:
Struct
InterCollision{}is a combination of algorithm from Collision Extension and Creation Kernel.FusionAlgorithm.hppdecides if fusion happens and chooses momenta of new particles.The setup that works and tests this extension is at: Fusion_Setups/DT-nHe_hal8999
Some preliminary results:


To do in consecutive PRs: