Skip to content

Superimposed element#591

Open
roussel-ryan wants to merge 19 commits intodesy-ml:masterfrom
roussel-ryan:superimposed
Open

Superimposed element#591
roussel-ryan wants to merge 19 commits intodesy-ml:masterfrom
roussel-ryan:superimposed

Conversation

@roussel-ryan
Copy link
Copy Markdown
Contributor

@roussel-ryan roussel-ryan commented Nov 13, 2025

This PR implements a SuperimposedElement class that supports placing an additional element in the center of another element.

Description

The SuperimposedElement class has a base_element and superimposed_element. During tracking the base_element is split in half and the superimposed_element is placed in between both halves at runtime. This allows modification of the base / superimposed element properties in place, without worrying about correctly accounting for the eventual splitting of the element.

Motivation and Context

This PR addresses problems with setting element attributes when trying to model beamlines that have superimposed elements. For example, previous cheetah lattices split the elements in the lattice itself, meaning that the length and strength elements were not equal to the full element properties. This created unexpected bugs when setting element parameters (e.g. geometric focusing strength) that depend on the length of the element (for example, when converting from integrated gradient to

Addresses #592

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

@roussel-ryan roussel-ryan mentioned this pull request Nov 17, 2025
14 tasks
@phys-cgarnier
Copy link
Copy Markdown

@cr-xu Hi, can I have the workflows approved?

@phys-cgarnier
Copy link
Copy Markdown

@cr-xu What should I do about the pkl file used for consistency test?

@phys-cgarnier
Copy link
Copy Markdown

@cr-xu Also, I don't know that it makes sense to implement splitting for the super imposed element unless splitting is grabbing the already split components in the superimposed element.

@cr-xu
Copy link
Copy Markdown
Member

cr-xu commented Apr 9, 2026

@cr-xu What should I do about the pkl file used for consistency test?

For a new element being implemented, you could just generate those and add those to the tests. They are mostly for keeping track that we don't accidentally introduce breaking changes.

@cr-xu
Copy link
Copy Markdown
Member

cr-xu commented Apr 9, 2026

@cr-xu Also, I don't know that it makes sense to implement splitting for the super imposed element unless splitting is grabbing the already split components in the superimposed element.

That probably depends on the way how it is intended to be used. From the current code I see that the superimposed element is put at the center of the other element. So if the real element is not centered, we would already split the base element accordingly and only construct the SuperImposed with split elements, right? In that case, the splitting doesn't make sense and you can just leave it empty.

@phys-cgarnier
Copy link
Copy Markdown

@cr-xu
I cannot set the PR to ready for review, would you mind doing this?

@cr-xu cr-xu marked this pull request as ready for review April 13, 2026 15:36
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.

3 participants