Intrabeam scattering tracking implementation
Hello Salah,
Nicely done! Let's use this interface to review your code. One major thing is that documentation is missing, but we can fix this later when you solve all the other requests. Here are my requests on your IBS implementation right now.
- (lines 13-15) You could move these imports inside your IBS class because they are only used by the IBS part of the code.
- (line 535) The number of partitions cannot be selected without modifying the source code.
- (line 536) I believe your self.dt is the same as self.ring.T0.
- (line 538) You can import the classical electron radius from scipy.constants.physical_constants. However, this should be more general. Consider a situation when the code is used to compute the IBS of a hadron/proton accelerator. In that case, would you still use classical electron radius in the IBS formulas?
- (line 541) There's no way to select the IBS model to compute T_ibs without modifying the source code.
- (line 551,552) Why not use the same assignment as in line 546 (beta functions).
- (lines 563-584) All three functions are the same. The only difference is their arguments. It should be a single function f (a, b, q), and you call it in your code f (a, b, q), f (1/a, b/a, q/a), and f (1/b, b/a, q/b). This will reduce the number of lines in the code and make it more readable.
- (line 604-612) This is tracking. It should be inside the track() function. Functions get_T_ibs() should compute the T_ibs.
- (line 621, 622) Same as (line 551, 552)
- (line 666-676) Same consideration as (line 563-584)
- (line 545, 614, 716, 770) These two functions share some similarities. You probably should create a separate function that does this shared part (like averaging different positions 's'). Then you call get_T_CIMP() or get_T_Piwinski() from it.
- (line 797-815) Same consideration as (line 563-584, line 666-676)
- (line 854) All the tracking related functionality (manipulation of x, xp, y, yp, delta, tau) and related 'kicks' should be implemented here. Model selection should be done when the object is initialised.
Merge request reports
Activity
requested review from @GUBAIDULIN
assigned to @FEDDAOUI
added 1 commit
- 451cbc74 - [bug] mbtrack2.utilities.optics.Optics.load_from_AT now works for lattices with any periodicity
added 37 commits
-
ec97993c...ad4e7890 - 36 commits from branch
develop
- 3c33d20a - Accepted changes from branch 'develop' into 'Ibs-implementation'
-
ec97993c...ad4e7890 - 36 commits from branch
- Resolved by GUBAIDULIN
- Resolved by GUBAIDULIN
- Resolved by GUBAIDULIN
- Resolved by GUBAIDULIN
Hi Salah, Looks good. There are a few things left to do here besides my specific threads above.
- There is some common functionality between all your functions for get_T_model> (). For example, in many (most) models, A is computed; in all models, you compute T_ibs at locations s and then average them. In principle, all common functionality like that should be written only once.
- Some things like H-functions are constant (depend only on Twiss alpha/beta/gamma) -> can be computed in init()
- You need to generate documentation for the class and all functions in numpy docs format. Also, by convention in mbtrack2 we add references in the documentation. So, you'd need to add references to all models that you implemented to papers where the model is described. (and the same for Zampetakis kick).
added 4 commits
-
e4bbee58...3a5cabb6 - 3 commits from branch
develop
- 87f607a9 - Accepted incoming changes from branch 'develop' into 'Ibs-implementation'
-
e4bbee58...3a5cabb6 - 3 commits from branch
- Resolved by GUBAIDULIN
- Resolved by GUBAIDULIN
- Resolved by GUBAIDULIN
- Resolved by GUBAIDULIN
- Resolved by GUBAIDULIN
- Resolved by GUBAIDULIN
- Resolved by GUBAIDULIN
Please register or sign in to reply