Skip to content

update dispersive laser #5484

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Oct 10, 2025
Merged

update dispersive laser #5484

merged 2 commits into from
Oct 10, 2025

Conversation

@mafshari64
Copy link
Contributor

@mafshari64 mafshari64 commented Sep 25, 2025

No description provided.

PrometheusPi
PrometheusPi requested changes Sep 25, 2025
View reviewed changes
lib/python/picongpu/picmi/lasers/dispersive_pulse_laser.py Outdated
[16, -16],
[16, -16],
],
picongpu_huygens_surface_positions: typing.List[typing.List[int]] = ([[16, -16], [16, -16], [16, -16]]),
Copy link
Member

@PrometheusPi PrometheusPi Sep 25, 2025

Choose a reason for hiding this comment

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

Does this follow our code style rules?

Copy link
Member

@PrometheusPi PrometheusPi Sep 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

And why is this set here, if this is also set (in exactly the same way) in the GaussianLaser init?
This looks like code duplication and should be avoided.

lib/python/picongpu/picmi/lasers/dispersive_pulse_laser.py
if duration <= 0:
raise ValueError(f"laser pulse duration must be > 0. You gave {duration=}.")
# Initialize standard Gaussian laser fields
super().__init__(**kw)
Copy link
Member

@PrometheusPi PrometheusPi Sep 25, 2025

Choose a reason for hiding this comment

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

How is the case handled with Laguerre modes?
It looks like that this code just takes them (as they are part of GaussianLaser), but will not treat them correctly in pypicongpu. I think this is not a valid solution.

Issue with:

  • picongpu_laguerre_modes
  • picongpu_laguerre_phases

Copy link
Member

@PrometheusPi PrometheusPi Sep 25, 2025

Choose a reason for hiding this comment

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

@steindev, you, as the ultimate authority on lasers 😜, what is your opinion?

@PrometheusPi PrometheusPi added refactoring code change to improve performance or to unify a concept but does not change public API CI:no-compile CI is skipping compile/runtime tests but runs PICMI tests PICMI pypicongpu and picmi related labels Sep 25, 2025
@PrometheusPi PrometheusPi added this to the 0.9.0 / next stable milestone Sep 25, 2025
PrometheusPi
PrometheusPi approved these changes Sep 26, 2025
View reviewed changes
Copy link
Member

@PrometheusPi PrometheusPi left a comment

Choose a reason for hiding this comment

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

requested changes were included
@steindev please comment/review and decide on merging

@mafshari64
Copy link
Contributor Author

mafshari64 commented Sep 26, 2025

@steindev, @PrometheusPi , @chillenzer
another solution can be in picmi side, define a file with common parameters and then Gauss and dispersive laser inherit from that.

chillenzer
chillenzer previously requested changes Oct 6, 2025
View reviewed changes
Copy link
Contributor

@chillenzer chillenzer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Good idea, thanks for the contribution.

Just blocking this, so we can wait for @steindev. Feel free to dismiss my review, @steindev, once you're done.

Sorry, didn't realise that there was already a solution for this.

@chillenzer
Copy link
Contributor

chillenzer commented Oct 6, 2025

Also: Have you tested that this works as expected? Test coverage for lasers is poor as of now. Not 100% how default_converts_to interacts with new setup. I think it should be fine but as long as tests are missing...

@chillenzer chillenzer dismissed their stale review October 6, 2025 09:45

Misinterpreted the status of this.

@chillenzer
Copy link
Contributor

chillenzer commented Oct 10, 2025

Any updates with respect to testing this?

@mafshari64
Copy link
Contributor Author

mafshari64 commented Oct 10, 2025 •
edited by PrometheusPi
Loading

I ran the main.py instead of picmi.GaussianLaser i used picmi.DispersivePulseLaser. I complied and then ran via tbg and saw the simoutput files.

laser_duration = 30.0e-15 / 2.354820045
laser_pulse_init = 20.0
laser = picmi.DispersivePulseLaser(
    wavelength = 800.e-9, 
    waist = 19.4e-6,
    duration = laser_duration,
    propagation_direction = [0.0, 1.0, 0.0],
    polarization_direction = [1.0, 0.0, 0.0],
    focal_position=[
        float(numberCells[0] * cellSize[0] / 2.0),
        1.05e-3,
        float(numberCells[2] * cellSize[2] / 2.0),
    ],
    centroid_position=[
        float(numberCells[0] * cellSize[0] / 2.0),
        -0.5 * laser_duration * laser_pulse_init * c,
        float(numberCells[2] * cellSize[2] / 2.0),
    ],
    picongpu_polarization_type = picmi.lasers.PolarizationType.LINEAR,
    a0 = 2.3127,
    phi0 = 0.0,
    #picongpu_gdd_si = 37700.0,
)

@chillenzer chillenzer merged commit 09f0a58 into ComputationalRadiationPhysics:dev Oct 10, 2025
10 checks passed
@PrometheusPi
Copy link
Member

PrometheusPi commented Oct 10, 2025

@mafshari64 and it works also with the commented out #picongpu_gdd_si = 37700.0 activated?

@mafshari64
Copy link
Contributor Author

mafshari64 commented Oct 10, 2025

I did not add but if you want I can rerun it again.

@PrometheusPi
Copy link
Member

PrometheusPi commented Oct 10, 2025

It would be great if you could test it, because right now, we would see no difference if it falls back to the default Gaussian.
Or you just validate the code in incidentField.param.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@PrometheusPi PrometheusPi PrometheusPi approved these changes

@chillenzer chillenzer chillenzer left review comments

Assignees

No one assigned

Labels

CI:no-compile CI is skipping compile/runtime tests but runs PICMI tests PICMI pypicongpu and picmi related refactoring code change to improve performance or to unify a concept but does not change public API

Projects

None yet

Milestone

0.9.0 / next stable

Development

Successfully merging this pull request may close these issues.

3 participants

@mafshari64 @chillenzer @PrometheusPi