-
Notifications
You must be signed in to change notification settings - Fork 24
Create a new device class for tcavs #261
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
Conversation
…sing two pvs for fb state
…ther tcavs are different
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.
Hi Chris, looks consistent with our other device classes. I found a bunch of places where pydantic validation could replace your manual validation. This device is unique because its configuration depends on its mode_config, but I think that's handled well here. Once we figure out the validation stuff.
lcls_tools/common/devices/tcav.py
Outdated
options = [options_to_check] | ||
for option in options: | ||
if option not in self.controls_information.mode_config_options: | ||
print( |
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 an error here would be more appropriate. As it stands, the check_options
decorator doesn't do anything to communicate to the code that something went wrong.
lcls_tools/common/devices/tcav.py
Outdated
|
||
def decorated(self, *args, **kwargs): | ||
if self.mode_config == "Disable": | ||
print("Unable to perform action, TCAV is in Disabled state") |
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.
Another place where an error might be better than a print statement.
|
||
@amp_set.setter | ||
@check_state | ||
def amp_set(self, amplitude): |
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.
Pydantic lets you validate function arguments with function annotations like so:
def amp_set(self, amplitude: float):
The if statement shouldn't be necessary.
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 was wrong about this, the annotations require you to wrap your functions with another decorator. Your current pattern of checking in place is fine.
lcls_tools/common/devices/tcav.py
Outdated
@amp_fbenb.setter | ||
@check_state | ||
def amp_fbenb(self, state: Union[str, int]): | ||
if not isinstance(state, str) or not isinstance(state, int): |
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.
Pydantic should already be validating this.
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.
Sorry, the expected type is actually an enum_str, I couldn't get the validation to work how I thought I could.
Unless I made the properties pydantic objects
lcls_tools/common/devices/tcav.py
Outdated
@phase_fbenb.setter | ||
@check_state | ||
def phase_fbenb(self, state: Union[str, int]): | ||
if not isinstance(state, str) or not isinstance(state, int): |
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.
Another place where pydantic covers function validation.
lcls_tools/common/devices/tcav.py
Outdated
@amp_fbst.setter | ||
@check_state | ||
def amp_fbst(self, state: Union[str, int]): | ||
if not isinstance(state, str) or not isinstance(state, int): |
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.
Same as above.
lcls_tools/common/devices/tcav.py
Outdated
@phase_fbst.setter | ||
@check_state | ||
def phase_fbst(self, state: Union[str, int]): | ||
if not isinstance(state, str) or not isinstance(state, int): |
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.
Same as above.
lcls_tools/common/devices/tcav.py
Outdated
controls_information: SerializeAsAny[TCAVControlInformation] | ||
metadata: SerializeAsAny[TCAVMetadata] | ||
|
||
def __init__(self, *args, **kwargs): |
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 don't think you need an __init__
function when using Pydantic
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.
Looks good in general. Some cleanup to be done with implementing Pydantic validation in some of the TCAV properties.
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.
Hi Chris, thanks for the thoughtful update. I like the new pattern for checking the amplitude and phase feedback options a lot better.
I was wrong when I said the validation should just require annotations like this def foo(bar: int)
. They need a decorator too, and it looks like we only do that in one place in our code:
lcls-tools/lcls_tools/common/devices/magnet.py
Lines 38 to 40 in fbaaa7f
@field_validator("*", mode="before") | |
def validate_pv_fields(cls, v: str) -> PV: | |
return PV(v) |
If you want to do validation still, I would suggest you just throw
ValueError
in the appropriate places. Either that or remove the silent failures.
Thank you!
) | ||
|
||
self._mode_config_options.update( | ||
{option: i for i, option in enumerate(mode_config_options["enum_strs"])} |
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 like this pattern better than the old one, nice.
lcls_tools/common/devices/tcav.py
Outdated
return setter_method(self, enum_str) | ||
return decorated | ||
return decorator | ||
""" |
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.
This can be removed I think.
|
||
@amp_set.setter | ||
@check_state | ||
def amp_set(self, amplitude): |
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 was wrong about this, the annotations require you to wrap your functions with another decorator. Your current pattern of checking in place is fine.
self.set_amplitude_feedback_options() | ||
self.setup_phase_feedback_option() | ||
|
||
def set_mode_config_option(self): |
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.
Maybe this should be called get_mode_config_option
?
} | ||
) | ||
|
||
def setup_phase_feedback_option(self): |
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.
This should be consistent with the above set_amplitude_feedback_option
too I think.
…ponding row in lcls_elements.csv
TCAV Device class:
Changed generator.py to include extract_tcav function. Also implemented advanced filtering in extract devices. You can pass an unpacked dictionary that is organized by {field_name: expected value}
For example :
additional_filter_constraints = {"Engineering Name": "TRANS_DEFL"}
Added relevant fields to metadata.py