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

Add space-charge to Cheetah #142

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

Conversation

greglenerd
Copy link

@greglenerd greglenerd commented Apr 3, 2024

This implements space-charge as outlined in #137

Description

Draft: todo

Motivation and Context

See #137

  • I have raised an issue to propose this change (required for new features and bug fixes)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (update in the documentation)

Checklist

  • I have updated the changelog accordingly (required).
  • My change requires a change to the documentation.
  • I have updated the tests accordingly (required for a bug fix or a new feature).
  • I have updated the documentation accordingly.
  • I have reformatted the code and checked that formatting passes (required).
  • I have have fixed all issues found by flake8 (required).
  • I have ensured that all pytest tests pass (required).
  • I have run pytest on a machine with a CUDA GPU and made sure all tests pass (required).
  • I have checked that the documentation builds (required).

Note: We are using a maximum length of 88 characters per line

@greglenerd greglenerd marked this pull request as draft April 3, 2024 22:42
@jank324 jank324 added the enhancement New feature or request label Apr 6, 2024
cheetah/accelerator.py Outdated Show resolved Hide resolved
greglenerd and others added 2 commits April 10, 2024 19:31
Co-authored-by: Remi Lehe <remi.lehe@normalesup.org>
cheetah/accelerator.py Outdated Show resolved Hide resolved
@@ -299,6 +307,356 @@ def defining_features(self) -> list[str]:
def __repr__(self) -> str:
return f"{self.__class__.__name__}(length={repr(self.length)})"

class SpaceChargeKick(Element):
"""
Simulates space charge effects on a beam.
Copy link
Author

Choose a reason for hiding this comment

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

these params might be changed: I was thinking of a way to integrate spacechargekicks as a setting (eg spacecharge = True) which would automatically build spacechargekick objects and incorporate them appropriately, as they don't represent any physical element of the accelerator. @cr-xu @jank324

Copy link
Member

@jank324 jank324 May 6, 2024

Choose a reason for hiding this comment

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

I think at some point in the future, we should think about creating a Effects class and make SpaceChargeKick a subclass of that. But right now this doesn't make sense yet.

Nevertheless, here is how I think we should integrate adding space charge to elements right now in a way that can easily be extended in the future:

Basically what I would do is to add a method to the Element class that looks something like this:

class Element:

    ...

    def with_space_charge(self, resolution: float = 0.01, *args, **kwargs) -> Segment:
        splits = self.split(resolution)
        splits_with_space_charge = # List of [split, sc, split, sc, ..., split, sc] ... probably itertools has a nice way to create this ... *args and **kwargs go into SpaceChargeKick
        return Segment(elements=splits_with_space_charge, name=f"{self.name}_with_space_charge")

This should end up looking similar to what @RemiLehe proposed in the very beginning. So if you do

Drift(length=0.5, name="my_drift").with_space_charge(resolution=0.25, nx=64)

you get

Segment(
    name="my_drift_with_space_charge",
    elements=[
        Drift(length=0.25), SpaceChargeKick(nx=64), Drift(length=0.25), SpaceChargeKick(nx=64)
    ],
)

This example is probably not quite correct in many ways, but I think it illustrates my idea.

The nice thing about doing it this way would be that it automatically works for all elements that implement split correctly, and that it would be relatively straightforward to extend this for other collective effects in the future.

Does this make sense?

Copy link

@ax3l ax3l May 22, 2024

Choose a reason for hiding this comment

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

An alternative thought for the low level implementation: One could also create sub- or mixing classes for thin (L=0) and thick (L>0) elements. For elements in ImpactX, we currently use these mixin classes, e.g. Drift vs. thin Multipole.

Space charge kicks could be thin elements :)

For the high level interface, I agree on the above syntax. You want to automatically slice this up, a property or method on how many slices on the sliced element is a nice user interface.

class SpaceChargeKick(Element):
"""
Simulates space charge effects on a beam.
:param length: Length of the element in meters.
Copy link
Author

Choose a reason for hiding this comment

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

As for now, time discretization is manually implemented through the lenght. If spacechargekick objects are built automatically, it might be interesting to play on this parameter.

def is_skippable(self) -> bool:
return False

def plot(self, ax: matplotlib.axes.Axes, s: float) -> None:
Copy link
Author

Choose a reason for hiding this comment

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

I need to understand better the plot functions of cheetah to implement something here. The only thing I know is there will be an issue given the fact that spacechargekick elements add 'artificial' lenght to the beam. @cr-xu @jank324

Copy link
Member

Choose a reason for hiding this comment

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

@greglenerd So I think we need to be careful with the length property. In all existing elements, length represents the physical length of the beam pipe and Cheetah actually relies on this assumption in some places. I would suggest that SpaceChargeKick.length = 0.0 all the time (because it doesn't add length to the beam pipe), and that the property of SpaceChargeKick currently named length is renamed to something like effect_length. Does that make sense?

Based on this, I think SpaceChargeKick could be plotted with something like plt.axvspan as a background highlight from s - effect_length to s.

Copy link
Collaborator

@RemiLehe RemiLehe left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this PR!

I added a few suggestions ; we can discuss them further offline.

cheetah/accelerator.py Outdated Show resolved Hide resolved
cheetah/accelerator.py Outdated Show resolved Hide resolved
cheetah/accelerator.py Outdated Show resolved Hide resolved
cheetah/accelerator.py Outdated Show resolved Hide resolved
Comment on lines 322 to 327
nx: Union[torch.Tensor, nn.Parameter,int]=32,
ny: Union[torch.Tensor, nn.Parameter,int]=32,
ns: Union[torch.Tensor, nn.Parameter,int]=32,
dx: Union[torch.Tensor, nn.Parameter]=3,
dy: Union[torch.Tensor, nn.Parameter]=3,
ds: Union[torch.Tensor, nn.Parameter]=3,
Copy link
Collaborator

Choose a reason for hiding this comment

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

@cr-xu @jank324
Could you comment on whether nx, ny, dx, dy etc. need to be tensors (instead regular ints and floats)?

Copy link
Member

Choose a reason for hiding this comment

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

Good question. I think their role is very similar to that of Screen.resolution. So I think no one will every want get the gradient with respect to any of them (?), meaning they don't have to be nn.Parameter. I think we could get away with making them int and float. But, in NumPy, if you interleave operations on NumPy types and Python types, computations slow down significantly. I don't know to what extend this is also an issue in PyTorch, but just to avoid it, I would make them all torch.Tensor.

cheetah/accelerator.py Outdated Show resolved Hide resolved
cheetah/accelerator.py Outdated Show resolved Hide resolved
cheetah/particles.py Outdated Show resolved Hide resolved
tests/test_space_charge_kick.py Outdated Show resolved Hide resolved
tests/test_space_charge_kick.py Outdated Show resolved Hide resolved
@jank324
Copy link
Member

jank324 commented May 6, 2024

Weirdly all GitHub Actions except for the docs build are currently not showing up on this PR. We need to keep an eye on this. It could just be a result of the merge conflicts with master, in which case we can deal with it once we get to that merge.

greglenerd and others added 2 commits May 6, 2024 10:11
Co-authored-by: Remi Lehe <remi.lehe@normalesup.org>
Co-authored-by: Remi Lehe <remi.lehe@normalesup.org>
@jank324 jank324 self-requested a review June 4, 2024 14:33
outgoing_beam = segment_space_charge.track(incoming)

# Final beam properties
sig_xo = outgoing_beam.sigma_x
Copy link
Member

Choose a reason for hiding this comment

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

This still needs to be done

sigma_yp=torch.tensor([2e-7]),
)
# Initial beam properties
incoming_particles0 = incoming_beam.particles
Copy link
Member

Choose a reason for hiding this comment

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

This also still need doing

# Final beam properties
incoming_particles1 = incoming_beam.particles

torch.set_printoptions(precision=16)
Copy link
Member

Choose a reason for hiding this comment

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

Why was this done? I think this line should probably be removed. I think it's not needed and tests shouldn't change global configs when they run.

3 + 2 * torch.sqrt(torch.tensor(2))
)
Nb = total_charge / elementary_charge
L = beta * gamma * kappa * torch.sqrt(R0**3 / (Nb * electron_radius))
Copy link
Member

Choose a reason for hiding this comment

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

I would avoid capital and single letter variable names ... I guess this should be section_length?

@@ -269,6 +269,140 @@ def from_twiss(
dtype=dtype,
)

@classmethod
def uniform_3d_ellispoid(
Copy link
Member

Choose a reason for hiding this comment

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

This is a duplicate of the method below.

I think @greglenerd pasted this in here while Chenran and I were still working on it, but it was since merged into master and this PR. So this copy of the method is no longer needed.

Note that the two copies are not identical. The lower one should be kept.

cheetah/accelerator/space_charge_kick.py Outdated Show resolved Hide resolved
charge * inv_cell_volume[:, None, None, None]
) # Normalize by the cell volume

def _integrated_potential(self, x, y, s) -> torch.Tensor:
Copy link
Member

Choose a reason for hiding this comment

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

Can we add type annotations?

"""

r = torch.sqrt(x**2 + y**2 + s**2)
G = (
Copy link
Member

Choose a reason for hiding this comment

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

I guess it's called G in the original method. Capital single letter variable names are not very pythonic and can sometimes be confusing. Though it might still be clearer if the name from the original paper is used. So it could alternatively be called integrated_potential? Or we just circumvent this by returning without naming it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good.

cheetah/accelerator/space_charge_kick.py Outdated Show resolved Hide resolved
)

# Initialize the grid with double dimensions
green_func = torch.zeros(
Copy link
Member

Choose a reason for hiding this comment

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

green_func is probably also a bit confusing here because it's not a function, but whatever that function return, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed. How about green_func_values?

cheetah/accelerator/space_charge_kick.py Outdated Show resolved Hide resolved
cheetah/accelerator/space_charge_kick.py Outdated Show resolved Hide resolved
cheetah/accelerator/space_charge_kick.py Outdated Show resolved Hide resolved
cheetah/accelerator/space_charge_kick.py Outdated Show resolved Hide resolved
cheetah/particles/particle_beam.py Outdated Show resolved Hide resolved
cheetah/particles/particle_beam.py Outdated Show resolved Hide resolved
Co-authored-by: Jan Kaiser <jan.kaiser.email@googlemail.com>
cheetah/accelerator/space_charge_kick.py Outdated Show resolved Hide resolved
tests/test_space_charge_kick.py Outdated Show resolved Hide resolved
@cr-xu
Copy link
Member

cr-xu commented Jun 4, 2024

There appears to be also random failing of CI due to numerical issues?
This probably should go away when running tests with more macroparticles.

@RemiLehe
Copy link
Collaborator

RemiLehe commented Jun 9, 2024

@cr-xu Yes, the tests where failing randomly, due to stochastic fluctuations in the initial particle distribution. In order to avoid this, I fixed the random seed in the test.

+ x * s * torch.asinh(y / torch.sqrt(x**2 + s**2))
+ x * y * torch.asinh(s / torch.sqrt(x**2 + y**2))
)
return G
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return G
return integrated_potential


# Create a new tensor with the doubled dimensions, filled with zeros
new_charge_density = torch.zeros(
(self.n_batch,) + new_dims, **self.factory_kwargs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
(self.n_batch,) + new_dims, **self.factory_kwargs
(self.batch_size,) + new_dims, **self.factory_kwargs

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

Successfully merging this pull request may close these issues.

None yet

5 participants