Bugfixes and improvements to emittance measurement class #218
Conversation
- add wait_time argument
Add meme to requs
|
Note: the failing test is unrelated to this PR so I think we can merge without it passing -- this was an issue previously so I don't know how to address |
There was a problem hiding this comment.
Hi Ryan, thanks for the PR. The changes look good to me. I left a couple minor comments about code comments. Feel free to merge when those are ready.
Should we remove the cryo tests for now? I'm not clear on what they're doing, and I don't know why they used random numbers. It leads to inconsistent tests, e.g. the latest 3.9 run failed where the 3.10 run passed running the same tests.
| @@ -36,21 +37,24 @@ class QuadScanEmittance(Measurement): | |||
| design_twiss: Optional[dict] = None # design twiss values | |||
| beam_sizes: Optional[dict] = {} | |||
There was a problem hiding this comment.
Could we note the units here? Thank you!
There was a problem hiding this comment.
Should also correct the above rmat comment to match the new 2x2x2 matrix size.
There was a problem hiding this comment.
Added documentation
|
@eloiseyang @lisazacarias @hmarts9 could y'all chat about what the issue is w/ the cryo tests? slack or here is fine. I just don't know what the details and want to loop Lisa in. |
|
@roussel-ryan feel free to merge after adding the comments @eloiseyang mentioned above. Thank you both! |
roussel-ryan
changed the title
|
|
||
| return results | ||
|
|
||
| def measure_beamsize(self): | ||
| """Take measurement from measurement device, | ||
| store beam sizes in self.beam_sizes""" | ||
| time.sleep(self.wait_time) |
There was a problem hiding this comment.
For later, perhaps this delay should be in the Magnet class scan method instead?
Add an init.py file to testing folder such that it runs the emittance measurement unit test, fixes several bugs in the QuadScanEmittanceMeasurement class and benchmarks the results to beam dynamics simulations