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 clone method for beams and elements #289

Merged
merged 40 commits into from
Dec 6, 2024
Merged

Conversation

Hespe
Copy link
Member

@Hespe Hespe commented Oct 28, 2024

Description

There is currently no reliable way to obtain copies of Beam or Element. This has lead to copy.deepcopy being used in some places which is troublesome in some instances (see the issues linked below).

Motivation and Context

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.

@Hespe Hespe self-assigned this Oct 28, 2024
@Hespe
Copy link
Member Author

Hespe commented Oct 28, 2024

Would be wise to merge this PR after #284 such that Beam.clone() can be marked abstract.

@Hespe
Copy link
Member Author

Hespe commented Oct 28, 2024

Looks like the TransverseDeflectingCavity is broken if its used within a Segment and not on its own. The problem seems to be that Segment.track() calls super.track(), that is Element.track(), if the Segment is skippable. However, Element.track() falls back to the transfer map, which is not implemented for the cavity.

@Hespe
Copy link
Member Author

Hespe commented Oct 28, 2024

The second test failure should be fixed once #288 is merged.

@Hespe
Copy link
Member Author

Hespe commented Oct 28, 2024

@jank324 There are some lines in the plotting tests that change context beyond their own scope:

ares.areamqzm1.k1 = 5.0
ares.areamqzm2.k1 = -5.0
ares.areamcvm1.k1 = 1e-3
ares.areamqzm3.k1 = 5.0
ares.areamchm1.k1 = -2e-3

Not sure if we should adjust that somehow or if its fine in this case.

@Hespe
Copy link
Member Author

Hespe commented Oct 28, 2024

Looks like the TransverseDeflectingCavity is broken if its used within a Segment and not on its own. The problem seems to be that Segment.track() calls super.track(), that is Element.track(), if the Segment is skippable. However, Element.track() falls back to the transfer map, which is not implemented for the cavity.

Raised this issue in #290

@jank324 jank324 added bug Something isn't working enhancement New feature or request labels Nov 7, 2024
@Hespe Hespe mentioned this pull request Nov 13, 2024
7 tasks
@jank324
Copy link
Member

jank324 commented Nov 20, 2024

#288 is merged now.

@Hespe
Copy link
Member Author

Hespe commented Nov 21, 2024

The plotting related failure is now fixed. This leaves only #290 as a blocker for this PR.

@Hespe Hespe marked this pull request as ready for review November 22, 2024 10:19
@Hespe Hespe requested a review from jank324 November 22, 2024 15:35
@jank324
Copy link
Member

jank324 commented Dec 4, 2024

@cr-xu Does this PR now also fix #298?

@jank324 jank324 requested a review from cr-xu December 4, 2024 15:50
@jank324
Copy link
Member

jank324 commented Dec 4, 2024

@Hespe I can't officially request a review, so here is an unofficial request.

@jank324 jank324 self-requested a review December 4, 2024 15:54
@cr-xu
Copy link
Member

cr-xu commented Dec 4, 2024

@cr-xu Does this PR now also fix #298?

Feature / method-wise yes.
I think we should make it clear in the documentation how these values are handled, but this can be done afterwards.

@Hespe
Copy link
Member Author

Hespe commented Dec 5, 2024

Looks good to me except for these two comments about potentially missing features in the clone implementation.

@cr-xu
Copy link
Member

cr-xu commented Dec 6, 2024

Apart from that, it's good to go from my side.

@jank324 jank324 merged commit 66d5847 into desy-ml:master Dec 6, 2024
8 checks passed
@Hespe Hespe deleted the clone-methods branch December 6, 2024 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

plot_reference_particle_traces crashes for non-leaf tensors Deepcopy in BPM prevents gradient calculation
3 participants