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

Fix Ionization Current #3398

Conversation

jtrojok
Copy link
Contributor

@jtrojok jtrojok commented Oct 20, 2020

There was an issue with the ionization current where there was a division by zero. When the electric field was zero (such as before a laser pulse), the ionization current was still called with ionization energy being zero. I fixed with an if-else-statement as you can see in the IonizationCurrent.hpp. The ionization current is now working (tested).

Copy link
Member

@sbastrakov sbastrakov left a comment

Choose a reason for hiding this comment

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

Thanks for the fix.

Just a small question / note. From the changed piece of code it is not obvious (to non-experts) that zero is not a legal value (as this piece does not have division). Perhaps this kind of a check can be moved to somewhere where it's more obvious, that is where the division actually happens? I am not sure if this is possible to implement, just to float an idea. If this is too hard or causes code duplication or there is some other limitation, it is fine to leave this logic as is, just perhaps document it in the comment.

@sbastrakov sbastrakov added bug a bug in the project's code component: core in PIConGPU (core application) labels Oct 20, 2020
@sbastrakov sbastrakov added this to the 0.6.0 / 1.0.0: Next Stable milestone Oct 20, 2020
@jtrojok
Copy link
Contributor Author

jtrojok commented Oct 20, 2020

I could also implement the fix in the JIonizationCalc.hpp. It would then return 0 when the ionization energy is zero. But I'd rather let it be the way it is now, because it is way faster right now than it would be if I implemented it in the JIonizationCalc.hpp. Right now nothing happens if the ionization energy is zero, but if I changed it, the assignment would assign a null current. I'd just add an explanatory comment.

@psychocoderHPC
Copy link
Member

Thanks for the fix.

Just a small question / note. From the changed piece of code it is not obvious (to non-experts) that zero is not a legal value (as this piece does not have division). Perhaps this kind of a check can be moved to somewhere where it's more obvious, that is where the division actually happens? I am not sure if this is possible to implement, just to float an idea. If this is too hard or causes code duplication or there is some other limitation, it is fine to leave this logic as is, just perhaps document it in the comment.

I agree with @sbastrakov. IMO this fix needs at least a comment above the if and the implementation f JIonizationCalc needs also a comment above the operator()

@jtrojok jtrojok force-pushed the feature-2020-09_ionizationCurrent branch from 1b63509 to 841011c Compare October 23, 2020 14:45
@jtrojok
Copy link
Contributor Author

jtrojok commented Oct 23, 2020

Added two comments as requested

@sbastrakov sbastrakov dismissed psychocoderHPC’s stale review November 2, 2020 11:21

The change was implemented

@sbastrakov sbastrakov merged commit 00eb751 into ComputationalRadiationPhysics:dev Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug a bug in the project's code component: core in PIConGPU (core application)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants