Skip to content

Sextupole implementation - #406

Merged
jank324 merged 35 commits into
masterfrom
sextupole
Apr 28, 2025
Merged

Sextupole implementation#406
jank324 merged 35 commits into
masterfrom
sextupole

Conversation

@jank324

@jank324 jank324 commented Apr 22, 2025

Copy link
Copy Markdown
Member

Description

Implements a sextuple magnet element.

Motivation and Context

Alternative potentially cleaner approach to #399. Would close #42. Closes #230. Related to #277.

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

@jank324 jank324 added the enhancement New feature or request label Apr 22, 2025
@jank324 jank324 mentioned this pull request Apr 22, 2025
@amylizzle amylizzle mentioned this pull request Apr 23, 2025
5 tasks
@jank324
jank324 requested a review from Copilot April 23, 2025 12:10

Copilot AI left a comment

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.

Pull Request Overview

This PR implements a new Sextupole element for particle accelerator simulations, including corresponding tests and integration into the element registry.

  • Introduces the Sextupole element with transfer map and tracking logic.
  • Adds tests comparing Sextupole tracking against Ocelot and equivalence to a drift when k2 is zero.
  • Updates element registration and changelog to include the new sextupole.

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/test_sextupole.py Adds tests comparing Sextupole tracking to Ocelot and drift behavior.
tests/test_elements.py Registers the Sextupole element with a default length value.
cheetah/track_methods.py Introduces a new second-order transfer matrix via base_tmatrix.
cheetah/accelerator/sextupole.py Implements the Sextupole element with transfer mapping and tracking.
cheetah/accelerator/drift.py Updates device/dtype handling using verify_device_and_dtype.
cheetah/accelerator/init.py Adds Sextupole to accelerator module exports.
cheetah/init.py Exports Sextupole as part of the core element suite.
CHANGELOG.md Documents the addition of the Sextupole element.
Comments suppressed due to low confidence (2)

cheetah/accelerator/sextupole.py:10

  • [nitpick] The docstring for Sextupole still contains placeholder TODOs for parameters such as 'k2', 'misalignment', and 'tilt'. Consider updating these docstrings to clearly describe the expected units and purpose of these parameters.
class Sextupole(Element):

tests/test_sextupole.py:61

  • While the tests verify the Sextupole against Ocelot and drift behavior, there is no test covering the misalignment functionality. Adding a test case with a non-zero misalignment would ensure the misalignment handling in the transfer_map is properly validated.
def test_sextupole_parameter_beam_particle_beam_agreement():

@jank324
jank324 requested review from Hespe and cr-xu April 23, 2025 12:13
Comment thread cheetah/track_methods.py Outdated
Comment thread cheetah/track_methods.py Outdated
Comment thread tests/test_sextupole.py Outdated
@Hespe

Hespe commented Apr 25, 2025

Copy link
Copy Markdown
Member

Note for the future: We might want to look for a general solution for tests that should be applied to most if not all element classes. Maintaining these lists by hand like for the dtype and clone tests is prone to forgetting newly added elements as seen above.

@jank324

jank324 commented Apr 25, 2025

Copy link
Copy Markdown
Member Author

Note for the future: We might want to look for a general solution for tests that should be applied to most if not all element classes. Maintaining these lists by hand like for the dtype and clone tests is prone to forgetting newly added elements as seen above.

Can you create an issue for that and link it here?

Comment thread cheetah/accelerator/sextupole.py Outdated
@Hespe

Hespe commented Apr 25, 2025

Copy link
Copy Markdown
Member

Note for the future: We might want to look for a general solution for tests that should be applied to most if not all element classes. Maintaining these lists by hand like for the dtype and clone tests is prone to forgetting newly added elements as seen above.

Can you create an issue for that and link it here?

I have created #416

@jank324
jank324 requested a review from Hespe April 25, 2025 13:24
@jank324
jank324 marked this pull request as ready for review April 25, 2025 13:24
@jank324

jank324 commented Apr 25, 2025

Copy link
Copy Markdown
Member Author

This PR is done, awaiting only review from @cr-xu, re-review from @Hespe and that the general GitHub runner trap: 6 issue is resolved such that we can actually run all the tests.

@cr-xu

cr-xu commented Apr 26, 2025

Copy link
Copy Markdown
Member

The PR looks mostly good to me, I didn't go through the T matrix line by line though. If I read correctly, this is using the mad calculation conventions? Maybe we should mention it in the comments.

Comment thread cheetah/accelerator/sextupole.py Outdated
Comment thread cheetah/accelerator/sextupole.py Outdated
Comment thread cheetah/accelerator/sextupole.py
Comment thread cheetah/track_methods.py Outdated
@jank324
jank324 merged commit de46739 into master Apr 28, 2025
@jank324
jank324 deleted the sextupole branch April 28, 2025 09:13
This was referenced Apr 28, 2025
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.

Add sextupole Implement more lattice elements for cheetah

4 participants