Skip to content

Transfer map caching#532

Merged
jank324 merged 18 commits intomasterfrom
cache-transfer-maps
Sep 10, 2025
Merged

Transfer map caching#532
jank324 merged 18 commits intomasterfrom
cache-transfer-maps

Conversation

@jank324
Copy link
Copy Markdown
Member

@jank324 jank324 commented Aug 7, 2025

Description

Add functionality to elements to cache transfer maps and save on compute time.

Motivation and Context

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

One of Cheetah's stated goals is speed.

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.

@jank324 jank324 mentioned this pull request Aug 7, 2025
@jank324 jank324 added the enhancement New feature or request label Aug 26, 2025
Comment thread cheetah/accelerator/drift.py Outdated
Comment on lines +57 to +62
if self._cached_first_order_transfer_map is None:
R = drift_matrix(length=self.length, energy=energy, species=species)

self._cached_first_order_transfer_map = R

return self._cached_first_order_transfer_map
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We could write a decorator that would do this for any property on beams and elements automatically, where it would invalidate all computed properties when a defining_feature is changed.

We had been thinking about something along these lines in #538.

@Hespe, do you have thoughts on this?

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.

Which other properties of Beam or Element are expensive to calculate and read often to justify caching? Especially for Beam, I would most properties to change on every tracking, so in which scenario would we benefit from caching some of its computed properties? With transfer_map, that should be constant for most elements along the lattice.

Copy link
Copy Markdown
Member

@Hespe Hespe Sep 4, 2025

Choose a reason for hiding this comment

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

It might be more effective to cache some intermediate computation artifacts, e.g. in the TWISS parameters. Since if you are polling $\beta_x$ you are likely also asking for $\beta_y$. But that would of course be manual work to implement, not a decorator.

@jank324
Copy link
Copy Markdown
Member Author

jank324 commented Sep 5, 2025

I benchmarked the speeds of the commit history in this PR tracking through the ARES lattice on my laptop.

benchmark_commits_plot

@jank324 jank324 requested review from Hespe and Copilot September 5, 2025 12:16
@jank324 jank324 marked this pull request as ready for review September 5, 2025 12:17
@jank324 jank324 mentioned this pull request Sep 5, 2025
14 tasks
@jank324
Copy link
Copy Markdown
Member Author

jank324 commented Sep 5, 2025

Partial PR for #538.

This comment was marked as outdated.

@jank324 jank324 requested a review from Copilot September 8, 2025 08:52
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements caching for first-order and second-order transfer maps in the accelerator elements, aiming to achieve significant speed-ups by avoiding redundant calculations.

  • Refactors the first_order_transfer_map and second_order_transfer_map methods to become caching wrappers
  • Renames the actual computation methods to _compute_first_order_transfer_map and _compute_second_order_transfer_map
  • Updates all test files to use the new .track() method syntax instead of calling elements directly

Reviewed Changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.

File Description
cheetah/accelerator/element.py Implements the caching infrastructure with cache invalidation when defining features change
cheetah/accelerator/*.py Updates all element classes to rename their transfer map methods to the private _compute_* variants
tests/test_quadrupole.py Updates test syntax from direct element calls to .track() method calls and improves variable naming
CHANGELOG.md Documents the performance improvement feature

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread cheetah/accelerator/element.py
Comment thread cheetah/accelerator/element.py
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@jank324
Copy link
Copy Markdown
Member Author

jank324 commented Sep 8, 2025

Reran the commit benchmarking on my MacBook again.

benchmark_commits_plot

@jank324
Copy link
Copy Markdown
Member Author

jank324 commented Sep 8, 2025

Repeat benchmarks again on my MacBook with slightly more informative plot.

benchmark_commits_plot

Copy link
Copy Markdown
Member

@Hespe Hespe left a comment

Choose a reason for hiding this comment

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

Looks good to me in principle. Only question would be if we need tests for the caching (or rather proper cache invalidation, I guess) but I am not sure how to do that in a systematic fashion.

Comment thread cheetah/accelerator/element.py
@jank324
Copy link
Copy Markdown
Member Author

jank324 commented Sep 8, 2025

Benchmark repetition on a cluster node for confirmation.

benchmark_commits_plot-3

@jank324 jank324 merged commit 000b990 into master Sep 10, 2025
10 checks passed
@Hespe Hespe deleted the cache-transfer-maps branch September 17, 2025 09:08
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.

3 participants