Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add i18 beamline definition #722
base: main
Are you sure you want to change the base?
add i18 beamline definition #722
Changes from all commits
6b77b83
6a7a986
c5110bd
95abf4a
407af41
3de5fca
40543ac
fb16c14
e807242
c734a1c
a5a80b6
e4d5a71
4fb0e21
490f5ee
50ade1e
1907492
1f96446
eb30b76
0726a08
b1442b9
1e52f97
c12d3ba
68bbbbe
2a96e88
4455702
4927133
cc8e5e2
8214942
5cb3cbb
03e1575
3a0ff4f
a16645f
3977039
14e31c8
cace9c9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this metadata checked or copied from i22? If the former 👍 if the latter, I'd rather have something that is obviously wrong than something that looks correct until the analysis comes back nonsense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: move these inside the dcm method to prevent namespace leaking
Check warning on line 206 in src/dodal/beamlines/i18.py
Codecov / codecov/patch
src/dodal/beamlines/i18.py#L206
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should: or rename the device factory method to make it clear which specific diode this is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're defining 2 Table classes, but then re-using the same one for both of these tables?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
??the PVs are different, this is lab-beamline relationship
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/DiamondLightSource/dodal/blob/cace9c93ac32b2c92e8e515821eebedad13b0000/src/dodal/devices/i18/table.py
https://github.com/DiamondLightSource/dodal/blob/cace9c93ac32b2c92e8e515821eebedad13b0000/src/dodal/devices/i18/thor_labs_stag.py
You are defining 2 classes, then re-using the same class for both instances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok thanks
Check warning on line 280 in src/dodal/beamlines/i18.py
Codecov / codecov/patch
src/dodal/beamlines/i18.py#L280
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: not used anywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you're redefining a SimDetector here rather than using the one in ophyd-async that uses the PatternGenerator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The motor field isn't being used at all- is the PatternGenrator meant to read the motor and change its output depending on the value of the motor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure of the details - what do you think @iain-hall ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with PatternGenerator - could be useful. Any simulated detectors would be generally be used for offline testing rather than on the beamline, and we have one already for the diode used in the undulator gap/lookup table generator scan. Maybe we should only have real beamline devices in beamlines/i18.py, and add simulated devices as needed for offline testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the motor fields could be used in a plan with dot notation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should lose the internal motor- you don't want to write your plans with the assumption that your Detector will have an internal motor, you want to write them to be generic enough to use StandardDetector: this will allow you to re-use the plans you're writing for your simulated detector. If you're not going to be using the position of the motor as an input to your PatternGenerator, I would remove it. If you are, I would make it an internal/private field.