-
Notifications
You must be signed in to change notification settings - Fork 3
[WIP] Implementation of the name based listing #12
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
|
One of the discussed format was Option 2 This avoids the wrapper class: line = Line(
line={
'drift1': drift1,
'quad1': quad1,
'drift2': drift2,
'quad2': quad2,
'drift3': drift3,
}
)kind: Line
line:
drift1:
kind: Drift
length: 0.25
quad1:
kind: Quadrupole
length: 1.0
MagneticMultipoleP:
Bn1: 1.0
drift2:
kind: Drift
length: 0.5
quad2:
kind: Quadrupole
length: 1.0
MagneticMultipoleP:
Bn1: -1.0
drift3:
kind: Drift
length: 0.5This can be implemented without any custom wrappers. |
|
Thanks for the PR! If the PALS contributors eventually agree on this "option 2", this seems like a good way to implement that. The only potential drawback I see is that the name of an element is not accessible anymore from the element instance (since On another note, the existing CI workflows needed approval to run (as this is your first contribution to this repository). After approval, you can see that the existing unit tests fail and need to be updated too, similarly to the FODO example. |
|
Hi @danielkallendorf, do you plan to make the changes that were discussed in the weekly meeting a few weeks ago (namely, add a |
| kind: Literal["Line"] = "Line" | ||
|
|
||
| line: List[ | ||
| line: OrderedDict[ |
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 love the idea of the PR, but I just noticed an issue with the data structure:
I think it does not work to make Line an ordered dict, or a dictionary at all.
The problem with dictionaries is that their elements are either unsorted (generally random, i.e. independent of name and insertion order because they are usually implemented as hash tables, where a key is hashed to determine which "bucket" an element belongs in) or sorted, by (key) name in our case, which is not what we want for a beamline/segment: we need a datastructure that strictly keeps the insertions order (across programming languages and formats). That format is a list.
As a specialty of Python, since 3.7 Python dicts preserve the insertation order, but that is not very portable to assume for other programming languages (or even YAML libraries).
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.
But what works is this:
- Line is a list
- Element is a dict (with only one key) of dict (the actual element properties)
- thingA:
kind: Sextupole
length: 3
- thingB:
kind: Quad
length: 1
- thingC:
length: 2that then looks in Python like this:
[{'thingA': {'kind': 'Sextupole', 'length': 3}}, {'thingB': {'kind': 'Quad', 'length': 1}}, {'thingC': {'length': 2}}]
and in a Python API it stays as now:
Sextupole(name="thingA", length=3)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.
Just for completeness, one cannot really do:
- thingA:
kind: Sextupole
length: 3
- thingB:
kind: Quad
length: 1
- thingC:
length: 2which represents
[{'thingA': None, 'kind': 'Sextupole', 'length': 3}, {'thingB': None, 'kind': 'Quad', 'length': 1}, {'thingC': None, 'length': 2}]
because then the random key that is the name is unclear how to pick out (again: order is not guaranteed, it might not be first)
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.
In your comment above, #12 (comment), did you have in mind that line would be a list of dict, so something like
line: List[
Dict[
str,
Annotated[
Union[
BaseElement,
ThickElement,
DriftElement,
QuadrupoleElement,
"Line",
],
Field(discriminator="kind"),
],
]
]or did you have in mind that we should implement some sort of new WrapperElement class (also derived from BaseModel, like all other element classes) that has a single attribute, a dictionary, which is the initialized to store one single "key: value" pair, where the value is the original dictionary of properties obtained by a model_dump from the a given element?
I have experimented a bit with the latter but it does not seem very easy to me. It also seems to require custom serialization and deserialization, which makes me think that we could do just that (custom serialization and deserialization, as done in #8) without the layer of the dictionary wrapper.
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.
Yes, line as a list of dict :)
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.
(with references to other pre-defined elements, it will then be a list of dict|str, but we can add references and inheritance of elements in a follow-up. The dict part or its attributes we could still derive from a base element.)
|
Replaced by #14 :) |
No description provided.