search

atcollab / at

Accelerator Toolbox
Apache License 2.0
48 stars 31 forks source link

Regression bug from PR #774: beam_current lattice attribute not propagated correctly #778

Closed swhite2401 closed 3 months ago

swhite2401 commented 3 months ago

This PR fixes a regression bug from #744. Modification in the lattice_object initialization resulted in wrong propagation of the _beam_current attribute. This was discovered when performing consecutive add_beamloading() calls on the same lattice. The _std_attribtues were modifed to propagate the attribute correctly, the specific case that was failing before is now working properly.

swhite2401 commented 3 months ago

@lcarver there is an issue with the rotate_table_history in atimplib.c

This dates from a while back.... most likely it was not thoroughly tested. I have restored the old version and it works now. There are some obvious differences in the way the buffers are written.

I have not time to finish tonight, this is just an update in case you want to have a look later today.

swhite2401 commented 3 months ago

@lcarver, the present version seems to be working fine but I haven't done extensive tests. In the end, I had to rotate the turnhistory buffer manually, maybe it could be simplified but it is not so important for the moment.

Could you please run you tests in various configurations? Could you make sure that the Vbeam, Vgen and Vbunch buffers that still use memmove are working properly? We may adopt the same method as in rotate_turnhistory.

Then we would need to add unit tests to make sure that this cannot break again. For this, I would suggest to track with various # of slices, turns and bunches and make sure the turnhistory, and voltage buffers rotate correctly.

swhite2401 commented 3 months ago

@lcarver I haver added a teste of the circulating buffers as discussed

lcarver commented 3 months ago

So this bug was highlighted by jimin seok who was encountering some problems. This PR solves the problem of the segmentation faults with Nturns>1.

I am truly shocked that this was somehow missed. Typically when making changes to any of the collective effects PassMethods, I run the example scripts to make sure they are all working. In this case, it was not done. The testing clearly needs to be improved so the tests proposed here are a good start. More tests of all CE or multi bunch related parameters will be put in in the future.

In the meantime, all example scripts are working well (and some other previously used scripts for beamloading calculations). Both files from Jimin are now running with no issues.

So for me it can be merged, but we should add more tests for these modules.

swhite2401 commented 3 months ago

@lcarver you suggestions were implemented, this is is ready for merge on your side?

lcarver commented 3 months ago

Yes, for me there was no real changes, I just added a few extra comments in one of the example scripts to make it clear that it was not doing the 'most efficient' way of coding. All good for me.