radiasoft / sirepo

Sirepo is a framework for scientific cloud computing. Try it out!
https://sirepo.com
Apache License 2.0
64 stars 32 forks source link

Changes to twiss_output command are not saved #3362

Open cchall opened 3 years ago

cchall commented 3 years ago

When I have just one twiss_output command changes to Twiss fields (beta's, alpha's, eta's) are not being saved. I tested a few other fields and they seemed to behave correctly.

If I create a second twiss_output command I can edit the Twiss fields there but deleting the first, misbehaving twiss_output causes edits to the remaining twiss_output command to be removed.

cchall commented 3 years ago

Has this been looked at yet? Not being able to easily update Twiss parameters makes it quite hard to do significant work with elegant on Sirepo.

moellep commented 3 years ago

I'll look at this today.

moellep commented 3 years ago

I see the problem. There was confusion with users setting the twiss values on the bunched_beam command (either on the source page or on the command editor) and the values not getting used in the simulation, so issue #3185 was created to keep the twiss_output and bunched_beam twiss values in sync. So if you edit the bunched_beam twiss parameters, the first twiss_output values are updated.

This isn't obvious in the UI and is also confusing because changing the first twiss_output twiss values doesn't save the values.

One improvement would be to have updates to the first twiss_output twiss values also sync to the bunched_beam. Alternatively, the first twiss_output twiss values could appear disabled, so no updates are possible from that editor.

@cchall let me know if changing the bunched_beam twiss values would work in your case, or if you have a preference on how to keep the bunch/twiss commands in sync.

cchall commented 3 years ago

Thanks for going through what is happening @moellep.

I'm really not sure what the best behavior is for this. My opinion for the ideal behavior is that twiss_output and bunched_beam shouldn't be synched at all. From a beam physics perspective: the Twiss parameters are a fundamental property of the lattice and the beam may or may not be matched into the lattice and should be set independently. elegant accommodates the fact that commonly the beam is matched into the lattice with the bunched_beam.use_twiss_command_values flag, so the Sirepo behavior is currently reversed from elegant's approach. This also has the implication that we are modifying the behavior that experienced elegant users are conditioned to expect.

However, given the confusion that drove this change is common I think may be reasonable to just keep the current mirroring of bunched_beam to twiss_output. I can edit bunched_beam or it looks like just taking it out also works.

If the current behavior is maintained I would recommend some adjustments: