Skip to content

Feature: Screen Charge Deposition#625

Open
cr-xu wants to merge 131 commits intomasterfrom
feature/screen-charge-deposition
Open

Feature: Screen Charge Deposition#625
cr-xu wants to merge 131 commits intomasterfrom
feature/screen-charge-deposition

Conversation

@cr-xu
Copy link
Copy Markdown
Member

@cr-xu cr-xu commented Mar 16, 2026

Description

  • Add general Cloud-in-Cell (CIC) method for 1,2, and 3D charge deposition.
  • Add cloud-in-cell method for Screen, that is differentiable and faster than the kde method.

Fixes #433

Also partially addresses #543

This PR merges @roussel-ryan 's fork https://github.com/roussel-ryan/cheetah/tree/diag0 into the master.

Motivation and Context

  • 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.

Comment thread cheetah/accelerator/screen.py Outdated
Comment thread cheetah/accelerator/screen.py Outdated
Comment thread cheetah/accelerator/screen.py Outdated
Comment thread cheetah/latticejson.py Outdated
Comment thread docs/requirements.txt Outdated
@cr-xu cr-xu requested a review from jank324 April 20, 2026 19:19
jank324 and others added 15 commits April 23, 2026 11:01
Co-authored-by: Copilot <copilot@github.com>
…nces

Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
@jank324
Copy link
Copy Markdown
Member

jank324 commented Apr 24, 2026

I just discovered a fairly significant issue with the implementation as is: Cheetah assumes that there can be any number of vector dimensions, and how many there are is unknown. However, as it stands, the Cloud-in-Cell implementation assumes exactly one vector dimensions, and, I believe, relies on Tensor's number of dimensions to determine how many histogram dimensions are needed. This is fundamentally incompatible with how Cheetah does it. Instead, the Cloud-in-Cell implementation should follow the interface of numpy.histogramdd.

@roussel-ryan
Copy link
Copy Markdown
Contributor

Are you sure about this? We've been doing arbitrary batch dimensions for gpsr using similar code.

@jank324
Copy link
Copy Markdown
Member

jank324 commented Apr 24, 2026

Well ... I just started checking how we might fix this. And I think it might actually be vector-dimension-agnostic, but the docstring and argument verification just say otherwise.

Comment thread cheetah/utils/cloud_in_cell.py Outdated
Comment on lines +14 to +17
:param positions: List or tuple of particle position tensors, each of shape
`(..., N)`, leading with optional vector dimension. `N` is the number of
particles. The length `d` of `positions` determines the dimensionality
(1D, 2D or 3D).
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this doesn't actually match what the function really does.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wait, let me look at it a little more in detail

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wait, let me look at it a little more in detail

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I changed it now to something that I believe reflects much better how it works (or should work). Interestingly, this is exactly backwards from what np.histogramdd does. I don't know if that's bad or irrelevant. I find the way we do it now intuitive as well.

jank324 and others added 2 commits April 24, 2026 10:40
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.

Use kernel with compact support in screen KDE

3 participants