desy-ml / cheetah

Fast and differentiable particle accelerator optics simulation for reinforcement learning and optimisation applications.
https://cheetah-accelerator.readthedocs.io
GNU General Public License v3.0
33 stars 13 forks source link

Vectorised simulations #116

Closed jank324 closed 5 months ago

jank324 commented 9 months ago

‼️ Do not merge before v0.6.2 has been released! This must go in a version after that.

Description

Makes all simulations in Cheetah vectorised, as you would expect in normal neural network model implementations.

Motivation and Context

Vectorised simulations should allow for much faster parallel simulation of beams and settings. This is very similar to how you might run a neural network with batches, and helps both speed-based and gradient-based applications.

Closes #100.

Types of changes

Checklist

Note: We are using a maximum length of 88 characters per line

jank324 commented 9 months ago

Here is a tricky thing to consider: What effect does it have on gradients when we conditionally set some value in a computation to a constant. Is that effect bad? If so, how do we fix it?

E.g. igamma2 in Drift.transfer_map.

jank324 commented 9 months ago

We also need to consider input dimension flexibility. For now, most of the reworked code probably assumes one input dimension for beams and one for settings. Maybe the final code can be more flexible, i.e. allow any number of input dimensions for both ... kind of like having n-dimensional batches.

jank324 commented 9 months ago

I've made the decision now, that the batch dimensions of the incoming beam and the settings must match. We should add a way to broadcast them, i.e. if a beam only has a single value or if a some elements only have single value, they are broadcast to the dimensions of the batched input. This should not be done if any of them has a different batch dimension, which should not be allowed and hence fail.

jank324 commented 9 months ago

Looking at the optimize_speed.ipynb example Notebook, it seems like this change currently slows down Cheetah between 2x and 4x for single samples. On the other hand, if you run with 1,000 samples, we are looking at a 40x to 55x speed improvement on an M1 Pro CPU. This would likely be more with more samples and/or on GPU.

jank324 commented 9 months ago

Some outstanding todos:

jank324 commented 9 months ago

This will probably be a v0.7 feature.

jank324 commented 7 months ago

The last commit solves #127.

@cr-xu we might need to look at this at some point. Small rounding errors sometimes result in the emittance computation taking sqrt of a negative number, or in an emittance of 0.0. I think both is not physically possible. For now this is solved with the clamping, but I wonder if there is another reason for this and the clamping only fixes the symptoms. Not at all urgent though.

cr-xu commented 7 months ago

The last commit solves #127.

@cr-xu we might need to look at this at some point. Small rounding errors sometimes result in the emittance computation taking sqrt of a negative number, or in an emittance of 0.0. I think both is not physically possible. For now this is solved with the clamping, but I wonder if there is another reason for this and the clamping only fixes the symptoms. Not at all urgent though.

At first sight, this is a price we pay for staying in the SI-Units. Emittances are just more commonly very small numbers, usually expressed in mm*mrad, etc.

Since now we can choose the dtype freely, could it be resolved with double-precision...? Can you provide a MWE where the emittance goes to zero or negative numbers?

jank324 commented 6 months ago

@cr-xu I think I messed up trying to merge your cavity fixes. Could you please have a look?

cr-xu commented 6 months ago

@cr-xu I think I messed up trying to merge your cavity fixes. Could you please have a look?

I believe it's properly fixed now.

jank324 commented 6 months ago

@cr-xu I have two things you might be able to answer:

jank324 commented 6 months ago

Okay ... what else needs to be done?

Should we update the cheetah-demos repo before merging this ... kind of as a further test before putting this on master or should we just go for it?

cr-xu commented 6 months ago

@cr-xu I have two things you might be able to answer:

  • I removed the voltage <= 0.0 case from the cavity code because I'm not sure if it makes sense to catch it in the vectorised example. Do you see any potential issue with this?

I think it's safe to do it, as one normally expects an accelerating voltage. For the edge cases (ERL..?) the voltage<=0 case here wouldn't be accurate anyway.

  • Do we have any tests that check for physical correctness of the cavity against known correct results or Ocelot or whatever else?

I don't think of one yet. But yes, it's generally helpful to bring in more tests in the future, instead of mostly relying on ARES-EA section.

cr-xu commented 6 months ago

I just updated the BO with Cheetah prior example in the cheetah-demo in a separate branch (see commit 9940072) The vectorized version works without any problem and shows a nice speed-up :) I think the demos can be merged later back to the main branch, once this PR is merged.

The broadcast functionality is really handy to use. 👍

jank324 commented 5 months ago

@cr-xu I'm getting the Merging is blocked message again. I would like to figure out this time what is causing it. The only restriction I could find in the settings is the one I set a long time ago that you cannot push directly to master without a PR. I think it has something to do with changes requested in a review not being resolved, so I requested a re-review from you to test this.

jank324 commented 5 months ago

Nevermind ... I fixed it. The branch locking option seems to have been the issue

jank324 commented 5 months ago

Okay ... I didn't really fix it, but I also don't get it.

The branch locking option seems to be what causes it to tell us that we cannot merge this PR. At first I didn't want to disable it because then I was able to push to master without a PR. It turns out though, that I can do that even when branch locking is turn on, presumably because I'm an admin. So I turned it off and now we can merge PRs again.

I don't like that I'm able to accidentally push to master and as far as I understand the current settings should prevent an admin to push to master ... but clearly they don't. I also googled this and it keeps pointing to an option "Include administrators", but I cannot seem to find that one. I feels a little bit like it may have been renamed or removed (?)

jank324 commented 5 months ago

Just for reference: Even with your approval, reactivating the branch lock blocks the merge.