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 #1052 Gaussian Beam Envelope 2D #1090

Merged

Conversation

ax3l
Copy link
Member

@ax3l ax3l commented Sep 9, 2015

Fix #1052 This fixes the underestimation of the amplitude of the laser implementations for Gaussian Beam (and based on it for pulse front tilted Gaussian Beams) in 2D3V simulations.

Calculations of the pre-factor are performed on the host-side and therefore a simple if avoids code duplication.

  • needs backport to master (after review & merge)

Fixed image:
gaussian_beam_2d_3d_fixed

@ax3l ax3l added bug a bug in the project's code component: core in PIConGPU (core application) affects latest release a bug that affects the latest stable release labels Sep 9, 2015
@ax3l ax3l added this to the Open Beta milestone Sep 9, 2015
@PrometheusPi PrometheusPi self-assigned this Sep 11, 2015
@@ -53,9 +53,11 @@ namespace picongpu
//gaussian beam waist in the nearfield: w_y(y=0) == W0
const float_64 w_y = W0 * sqrt( 1.0 + ( FOCUS_POS / y_R )*( FOCUS_POS / y_R ) );
Copy link
Member

Choose a reason for hiding this comment

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

could you cast W0 to float_64 here?
or better
remove W0 * ... here and float_64( W0 ) / w_y in both cases below

Copy link
Member Author

Choose a reason for hiding this comment

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

out of scope for this fix (do not mix refactoring with bug fixes). it should be as minimal as possible, as every bug fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

I also need to backport it and it is already creating merge conflicts in the current version.

Copy link
Member Author

Choose a reason for hiding this comment

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

also, it is not necessary to cast W0. this line does a list of float calculations and casts the result to float_64 for the following operations.

Copy link
Member Author

Choose a reason for hiding this comment

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

opened in #1095

@ax3l
Copy link
Member Author

ax3l commented Sep 11, 2015

fun fact: I also did intentionally not update the year since this creates merge conflicts, too.

float_64 envelope = float_64( AMPLITUDE );
if( simDim == DIM2 )
envelope *= math::sqrt( float_64( W0 ) / w_y );
if( simDim == DIM3 )
Copy link
Member

Choose a reason for hiding this comment

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

just to improve readability, please change if to else if
as used in the rest of the code

Copy link
Member

Choose a reason for hiding this comment

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

thx

This fixes the underestimation of the amplitude of the
laser implementations for Gaussian Beam (and based on it
for pulse front tilted Gaussian Beams) in 2D3V simulations.

Calculations of the pre-factor are performed on the host-side
and therefore a simple `if` avoids code duplication.
PrometheusPi added a commit that referenced this pull request Sep 11, 2015
@PrometheusPi PrometheusPi merged commit dc09f42 into ComputationalRadiationPhysics:dev Sep 11, 2015
@ax3l
Copy link
Member Author

ax3l commented Sep 11, 2015

thx for the review!

@ax3l ax3l deleted the fix-GaussianBeam2D3V branch September 11, 2015 13:14
psychocoderHPC added a commit that referenced this pull request Sep 17, 2015
@ax3l ax3l mentioned this pull request Sep 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects latest release a bug that affects the latest stable release 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.

None yet

2 participants