add `BmadQuadrupole` element

jp-ga commented 1 month ago

Description

DRAFT PR add BmadQuadrupole element by interfacing with Bmad-X tracking routines.

Motivation and Context

closes #139

[x] 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)
[x] 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 commented 1 month ago

As a general comment. I'm not sure I like this quick-and-dirty drop-in way of integrating Bmad-X elements. However, I think it makes sense to test them this way. In the long-term we should think about how this can be clean up better.

Some questions we should ask ourselves:

Do we really want a separate bmadx module? This means that code is suddenly in a very different place than the other tracking code.
Should there be a single Quadrupole class that has something like a tracking_method: Literal["linear", "bmadx"] argument?

Maybe slightly unrelated but I saw it in this PR:

I think it's not very nice to have something like C = scipy.constants.speed_of_light at the top of a file. Ideally, one would just from scripy.constants import speed_of_light and use that. I think this might cause issues sometimes, when a torch.Tensor is strictly needed, but I cannot produce an example right now. Maybe we should add a cheetah.constants module that takes scripy.constnts and always returns torch.Tensors. But again, I'm not sure we actually need that ... just something to think about.

jp-ga commented 2 weeks ago

One small thing to keep in mind, bmadx tracking agrees to single precision with bmadx. I was expecting it to agree to double precision, but it is good enough for now.

jank324 commented 1 week ago

@jp-ga, the question came up with @roussel-ryan, if this or #163 should be merged first. I think a main question in that was if the changes in #163 might make conversions in this PR redundant. Do you have an opinion on this?

jp-ga commented 1 week ago

@jp-ga, the question came up with @roussel-ryan, if this or #163 should be merged first. I think a main question in that was if the changes in #163 might make conversions in this PR redundant. Do you have an opinion on this?

The transformations in this PR already assume the new canonical coordinates described in #163, so you can merge #163 first if needed.

desy-ml / cheetah