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

Change line 235 in gaussian.rs under laser, resolve energy not conser… #93

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

YijunTangCambridge
Copy link

Identified an error in gaussian.rs line 235, where I found the calculation of vector there is not correct. When ellipticity is defined in gaussian.rs approach by redefining x and y the calculation of intensity gradient will have to include extra factor dx'/dx as in the attached photo.

This issue is spotted when I investigate energy dynamic of a single atom, check kinetic energy, potential energy and total energy, found out that energy is not conserved for non-zero ellipticity. After imposing the changes to line 235 I redo energy conservation check, energy are conserved, comparison are shown in the other photon.

elliptic

elliptic2

…ve issue due to neglectance of chain rule in force gradient calculations
@ElliotB256
Copy link
Collaborator

Thanks for the report, I'll double check this but it looks good at first glance!

@ElliotB256
Copy link
Collaborator

If possible, could you define a unit test that fails for the earlier broken code and passes for your correct code? That would help to improve the coverage and prevent it occuring again.

@YijunTangCambridge
Copy link
Author

Sounds good, I will look up what tests were there already and see what I can do.

@YijunTangCambridge
Copy link
Author

Hi Elliot, I have now made modification on existing unit tests (there are 3 wrong unit test when I test the clean version, all about dipole force), and I added a new one to test the ellipticity issue, now there are no error from my end, could you check and proceed to approval if good? Many thanks :)

@YijunTangCambridge
Copy link
Author

  1. Correct unit test: laser::gaussian::tests: test_get_gaussian_beam_intensity_gradient

  2. Added and tested correct new unit test: test_get_elliptic gaussian_beam_intensity_gradient

  3. Correct unit test: laser::intensity_gradient::tests::test_sample_laser_intensity_gradient_again_system

  4. Correct unit test: dipole::force::tests::test_apply_dipole_force_and_gradient_system

I just summarize the changes I made

@YijunTangCambridge
Copy link
Author

Hi Elliot, just a reminder about my ellipticity fix :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants