-
Notifications
You must be signed in to change notification settings - Fork 3
Fix BeamLine name attribute #16
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
examples/fodo.py
Outdated
| yaml_data = yaml.safe_load(file) | ||
| # Parse YAML data | ||
| loaded_line = Line(**yaml_data) | ||
| loaded_line = Line(**yaml_data[0]) |
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.
yaml_data was a dictionary before, now it's a one-element list. Keeping the previous API for the Line constructor requires more work, which could be done in this PR or in a separate PR. I have not tried it yet (i.e., I don't know yet if I can find a way to make it work).
|
@EZoni does this need a rebase? |
|
Probably yes, after the last few commits in |
|
Conflicts fixed. |
examples/fodo.py
Outdated
| yaml_data = yaml.safe_load(file) | ||
| # Parse YAML data | ||
| loaded_line = BeamLine(**yaml_data) | ||
| loaded_line = BeamLine(**yaml_data[0]) |
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 makes me think, also for our current layout/draft:
- should a PALS file support having multiple beamlines (segments: yes)?
- if not, which one is the "entry point" to use by default? The 0-th element might be a lattice element and not a
kind: BeamLineonce we support inheritance
Ignoring the entry point question, we might need a "wrapping" LatticeElements class here in Python that can load a list of elements (instead of only loading [0] and assuming it is of kind BeamLine)
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 pushed a change in 464020d to return a dict instead of a single-element list.
I cannot remember where the single-element list came from, but the new change seems to avoid the API issue. Am I overlooking something?
I'm not sure I understand what the LatticeElements class should be, but maybe it could be done in a separate PR, unless it turns out that it's necessary here.
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.
If the problem was with the serialization of single elements, maybe it should be fixed separately as well.
|
From the PR description:
This is what I found: The pydantic serializer is how we go from pydantic classes to dicts / json and back. This is needed when we want to rename fields or do custom conversions that are serialized slightly different than the internal representation. It looks to me like this is primarily for output formatting changes, not input. The pydantic validator is used for custom validation logic, e.g., when someone sets a field/value/property, and can optionally modify values. But it seems this is also used for input changes... |
ax3l
left a comment
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.
Excellent, thank you!
Overview
With the implementation in the main branch, if we add a
nameattribute to thelineobject created in the FODO example, the attribute is ignored and the output isThis is not consistent with the example in https://github.com/campa-consortium/pals/blob/main/examples/fodo.pals.yaml.
With the implementation in this branch, if we add a
nameattribute to thelineobject created in the FODO example, the attribute is stored and the output iswhich is consistent with the example in https://github.com/campa-consortium/pals/blob/main/examples/fodo.pals.yaml.
This is achieved primarily by deriving the
Line(BeamLineafter #15) class from the base element classBaseElementinstead of deriving it from Pydantic's base model classBaseModel.I also merged the custom field validation of the
linefield with the custom model validation, since we needed one anyways to fix the deserialization. I don't see why they should be separate.I wonder if the right way to do this is using
@model_serializerinstead of@model_validator...To do