Add resistive wall Wakefields#111
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a comprehensive resistive wall impedance module (ResistiveWallImpedance) that computes longitudinal impedance Z(k) and wakefield W(z) through direct numerical integration. This complements the existing fast pseudomode approximation (ResistiveWallWakefield) by providing high-accuracy calculations suitable for validation and research applications.
Key changes:
- New
ResistiveWallImpedanceclass supporting flat and round geometries with direct impedance/wakefield calculations - Enhanced
ResistiveWallWakefieldwith lazy matplotlib imports, improved__repr__, and standardized validation - Added
wakefield_plot()visualization function to ParticleGroup and plot module - Updated documentation with two example notebooks and navigation entries
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pmd_beamphysics/wakefields/resistive_wall_impedance.py | New module implementing numerical impedance/wakefield calculations with AC conductivity support |
| pmd_beamphysics/wakefields/resistive_wall.py | Existing pseudomode wakefield implementation with improved documentation and lazy imports |
| pmd_beamphysics/wakefields/init.py | Module initialization exporting both wakefield classes and utility functions |
| pmd_beamphysics/plot.py | Added wakefield_plot() function and enhanced density_plot() with flexible axis support |
| pmd_beamphysics/particles.py | Added wakefield_plot() method to ParticleGroup class |
| mkdocs.yml | Added wakefields section to documentation navigation |
| docs/examples/wakefields/resistive_wall_wakefield.ipynb | Tutorial notebook for pseudomode wakefield usage |
| docs/examples/wakefields/resistive_wall_impedance.ipynb | Tutorial notebook comparing numerical and pseudomode approaches |
| docs/examples/data/SLAC-PUB-10707-digitized-Fig8-AC-Cu.csv | Reference data for flat geometry validation |
| docs/examples/data/SLAC-PUB-10707-digitized-Fig4-AC-Cu.csv | Reference data for round geometry validation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
ChristopherMayes
changed the title
|
A warning from pytest on my own desktop and in the github test runner:
|
|
|
MichaelEhrlichman
left a comment
MichaelEhrlichman
left a comment
There was a problem hiding this comment.
Nice addition to openPMD-beamphysics!
There was a problem hiding this comment.
Perhaps add a header, even a simple one.
There was a problem hiding this comment.
Perhaps add a header, even a simple one.
| "ResistiveWallWakefieldBase", | ||
| "ResistiveWallWakefield", | ||
| "ResistiveWallPseudomode", | ||
| "pseudomode", # Legacy alias |
There was a problem hiding this comment.
If the code is all new, there shouldn't really be legacy stuff, right? If this was from previous version, consider removing before merge so you don't have to support it for the rest of time.
There was a problem hiding this comment.
Thanks, removed.
pmd_beamphysics/wakefields/base.py
Outdated
| """ | ||
| return self.wake(-1e-15) | ||
|
|
||
| def apply_to_particles( |
There was a problem hiding this comment.
This is Euler's method for one step and only for pz. I am worried about not consistently changing the other coordinates, and also since this is ODE solving, the interface for the function really locks you in to having a basic method. I'm also not sure that ODE solving should be "owned" by the force class.
If it was me, I would consider moving ODE solving to it's own place in the code (if it's something openpmd-beamphysics should implement) with an interface where it could be expanded with more advanced methods down the line or remove from the class since .particle_kicks already exists and you can let users to the more ad-hoc pz+=wake*len on their own without building it into the code.
Alternatively, you have scipy as a dependency already and could wrap scipy.integrate in a way to accept ParticleGroup objects and force objects.
There was a problem hiding this comment.
This is mostly for analysis convenience. I don't plan on implementing solvers in this package. Note that simply multiplying by a length is what is often done in practice. For example. at LCLS the resistive wall wake is applied in one place with a 1 km length! I can add a note that this is simple.
pmd_beamphysics/wakefields/base.py
Outdated
|
|
||
| Parameters | ||
| ---------- | ||
| particle_group : ParticleGroup |
There was a problem hiding this comment.
This is also the only place in the WakefieldBase interface that uses ParticleGroup. Taking it out fully decouples the two classes and simplifies your interface.
There was a problem hiding this comment.
Removed. Instead now we have ParticleGroup.apply_wakefield(wakefield: WakefieldBase, length: float)
There was a problem hiding this comment.
I skimmed this and it appears to have a lot of duplicate functions from pmd_beamphysics/wakefields/resistive_wall.py and also isn't imported by anything. Is this leftover from some refactoring?
There was a problem hiding this comment.
I did a major refactor, removing a lot of redundant code.
| else: # quad | ||
| return wakefield_from_impedance(z, self.impedance, k_max=k_max) | ||
|
|
||
| def convolve_density( |
There was a problem hiding this comment.
Some of these functions are duplicated across the two resistive wall classes and could be promoted to the parent class for DRY code.
There was a problem hiding this comment.
This should be cleaned up with the latest refactor.
pmd_beamphysics/wakefields/base.py
Outdated
| return self._wakefield_func(z) | ||
|
|
||
| # Numerical cosine transform | ||
| from scipy.integrate import quad |
There was a problem hiding this comment.
Move imports to top of file
There was a problem hiding this comment.
This file is quite long, consider breaking out the two concrete wakefield implementations into own file.
There was a problem hiding this comment.
This file is also very long, could also break out concrete classes? Github is lagging badly rendering the diffs even without having an existing file to compare to.
Maybe:
wakefields/
base.py
impedance.py
psuedomode.py
tabular.py
resistive_wall/
base.py
psuedomode.py
impedance.py
| if return_figure: | ||
| return fig | ||
|
|
||
| def wakefield_plot( |
There was a problem hiding this comment.
Not the worst thing, but I will point out that this is the only method coupling ParticleGroup with the wakefields module. Removing this just means the user calls
wakefield_plot(pg, wf)
instead of
pg.wakefield_plot(wf)
but reduces coupling between the sections of the code.
There was a problem hiding this comment.
The reason that I don't do that is because it requires another import.
Co-authored-by: Christopher M. Pierce <contact@chris-pierce.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| from ..base import WakefieldBase | ||
|
|
||
|
|
||
| class Geometry(str, Enum): |
There was a problem hiding this comment.
My thought was more like this:
class Geometry:
...
class GeometryRound(Geometry):
radius: float
def W0(self):
return c_light * Z0 / (np.pi * self.radius**2)
class GeometryFlat(Geometry):
gap: float
def W0(self):
return c_light * Z0 / self.gap**2 * np.pi / 4
That way the parameter names are specialized to the type of geometry and potentially more complicated things that need stuff more than a radius could be accomodated later on.
Then
class ResistiveWallWakefieldBase(WakefieldBase):
conductivity: float
relaxation_time: float
geometry: Geometry = GeometryRound()
def W0(self):
return self.geometry.W0()
There was a problem hiding this comment.
Okay, but then the user would have to either import these classes, or we'd do some post init. I don't really want the user to have to import extra things in order to instantiate the wakefield.
pmd_beamphysics/wakefields/base.py
Outdated
| ax : matplotlib.axes.Axes, optional | ||
| Axes to plot on. If None, creates a new figure. | ||
| """ | ||
| import matplotlib.pyplot as plt |
There was a problem hiding this comment.
There's some places where imports are inside functions. Preferably they should be moved to top of file to make dependencies explicit.
New Classes
ResistiveWallWakefieldfrom_material("copper-slac-pub-10707", radius=2.5e-3)ResistiveWallPseudomodeto_bmad()ImpedanceWakefieldPseudomodeWakefieldGeometryEnumGeometry.ROUNDorGeometry.FLAT"round","flat")Shared Wakefield API
All wakefield classes inherit from
WakefieldBaseand share a consistent interface:wake(z)impedance(k)convolve_density(ρ, dz, plot=False)particle_kicks(z, weight)plot()plot_impedance()ParticleGroup Integration
New methods added to
ParticleGroup:apply_wakefield(wake, length, inplace=False)wakefield_plot(wake)Low-Level Functions
Exposed in
pmd_beamphysics.wakefields.resistive_wall:ac_conductivity()— Drude model AC conductivitysurface_impedance()— Surface impedance for conducting walllongitudinal_impedance_round()/longitudinal_impedance_flat()— Direct impedance calculationwakefield_from_impedance()— Wakefield via quadraturewakefield_from_impedance_fft()— Wakefield via FFT (fast)characteristic_length()— Characteristic length scale s₀krs0_round()/krs0_flat()— Polynomial fits for k_r·s₀Qr_round()/Qr_flat()— Polynomial fits for Q_rDocumentation
New example notebooks in
docs/examples/wakefields/:resistive_wall.ipynbimpedance_wakefield.ipynbImpedanceWakefieldReferences