Conversation
… into add-bumps-to-commisslib
|
Some changes still need to be tested during machine studies shifts, but I believe this first version is ready to be merged. |
|
Thanks for the review! In our next machine study (Monday), I will use some time to test the scans. After this, some other bugs will be corrected, and I think the class will be ready to be merged |
| def do_measurement(): | ||
| """Measurement function. | ||
|
|
||
| Change this function by an external | ||
| function that does the measurement | ||
| you want to perform at each bump point. | ||
| """ | ||
| print('Not a measurement!') | ||
| return {} |
There was a problem hiding this comment.
I don't understand. why is this method not implemented?
There was a problem hiding this comment.
It is because it can be a general measurement, it could be with XBPMS or other things. The user must define it in the script
apsuite/commisslib/bumps_scans.py
Outdated
| def update_reforb(self, refx, refy): | ||
| """Update reforb. | ||
|
|
||
| Args: | ||
| refx (1d numpy array): Horizontal orbit | ||
| refy (1d numpy array): Vertical orbit | ||
| """ | ||
| self.reforbx = refx | ||
| self.reforby = refy |
There was a problem hiding this comment.
I think this shouldn't exist anymore, right?
| def get_sofb_bpm_enbl(self): | ||
| """.""" | ||
| self._bpmxenbl = _np.copy(self.devices['sofb'].bpmxenbl) | ||
| self._bpmyenbl = _np.copy(self.devices['sofb'].bpmyenbl) | ||
|
|
||
| def get_fofb_bpm_enbl(self): | ||
| """.""" | ||
| self._fofb_bpmxenbl = _np.copy(self.devices['fofb'].bpmxenbl) | ||
| self._fofb_bpmyenbl = _np.copy(self.devices['fofb'].bpmyenbl) |
There was a problem hiding this comment.
I don't understand. Is this supposed to get an initial state of the enabled list? Shouldn't this run at the beginning of the experiment, instead of at the object initialization? shouldn't this the saved as metadata of the experiment?
There was a problem hiding this comment.
This represents the initial state of the enabled list. I believe we can store this state during object initialization. I don’t see it as a necessary parameter to include in the experiment metadata, but if you think it should be saved there, I can modify it accordingly.
apsuite/commisslib/bumps_scans.py
Outdated
| sofb.refx = self.params.reforbx | ||
| sofb.refy = self.params.reforby |
There was a problem hiding this comment.
Shouldn't this return to the reference orbit both systems had prior to the experiment, instead of the reference orbit used during the experiment?
There was a problem hiding this comment.
Yes, it is already doing that. The reference orbit prior to the experiment is stored in the Params class, and it is exactly the one being used here.
apsuite/commisslib/bumps_scans.py
Outdated
| ) | ||
| fofb.bpmxenbl = enblx | ||
| fofb.bpmyenbl = enbly | ||
| _time.sleep(0.5) # NOTE: For some reason We have to wait here. |
There was a problem hiding this comment.
what happens if you do not wait? isn't it possible to wait for something to happen and then move on? Fixed wait times like this one are not a good practice. We should avoid it.
There was a problem hiding this comment.
Sometimes the BPM enabled list was not being set correctly. After adding this wait, the issue has not occurred again.
apsuite/commisslib/bumps_scans.py
Outdated
| idcx = idx[:2] | ||
| idcy = idx[2:] - 160 | ||
| dorbx = self.devices['sofb'].orbx - refx | ||
| dorby = self.devices['sofb'].orby - refy | ||
| dorbx = dorbx[idcx] | ||
| dorby = dorby[idcy] | ||
| return _np.sqrt(_np.sum(_np.hstack([dorbx, dorby] ** 2))) |
There was a problem hiding this comment.
maybe
| idcx = idx[:2] | |
| idcy = idx[2:] - 160 | |
| dorbx = self.devices['sofb'].orbx - refx | |
| dorby = self.devices['sofb'].orby - refy | |
| dorbx = dorbx[idcx] | |
| dorby = dorby[idcy] | |
| return _np.sqrt(_np.sum(_np.hstack([dorbx, dorby] ** 2))) | |
| sofb = self.devices['sofb'] | |
| ref = np.r_[refx, refy] | |
| orb = np.r_[sofb.orbx, sofb.orby] | |
| dorb = (orb - ref)[idx] ** 2 | |
| return _np.sqrt(dorb.sum()) |
apsuite/commisslib/bumps_scans.py
Outdated
| dorby = dorby[idcy] | ||
| return _np.sqrt(_np.sum(_np.hstack([dorbx, dorby] ** 2))) | ||
|
|
||
| def set_orb(self, orbx, orby): |
apsuite/commisslib/bumps_scans.py
Outdated
| if self.params.use_fofb: | ||
| kick = _np.max(( | ||
| _np.abs(fofb.kickch_acc), | ||
| _np.abs(fofb.kickcv_acc), | ||
| )) | ||
| else: | ||
| _ = sofb.correct_orbit_manually( | ||
| nr_iters=nr_iters, residue=residue | ||
| ) |
There was a problem hiding this comment.
I think the meaning of use_fofb is obscure, because it not only controls whether FOFB will be used or not, it is also selecting whether the manual orbit correction with SOFB will be applied. Consider changing the name of the property to something more accurate.
apsuite/commisslib/bumps_scans.py
Outdated
| def save_data(self, fname, overwrite=False, compress=False): | ||
| """.""" | ||
| return super().save_data(fname, overwrite, compress) |
There was a problem hiding this comment.
Why re-implement this method?
… into add-bumps-to-commisslib
fernandohds564
left a comment
fernandohds564
left a comment
There was a problem hiding this comment.
@Gabrielrezende-asc and I talked offline about some suggestions. I'll wait for a new version for revision.
4 participants
No description provided.