-
Notifications
You must be signed in to change notification settings - Fork 3
Comprehensive Element Update #33
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
a9c3c05 to
3738740
Compare
Based on the prompt > We defined a meta-data schema for particle accelerator lattice lements in pals. > You will find in pals/source the description of elements and their meta-data, the pals/examples has an example. > Ignore the inheritance feature for now. > I want you to find and read the element definitions. > I want you to find and read the implementation of existing PALS Python elements in pals-python/. Follow their structure and style. > Then, do this workflow: > 1. read and understand one element at a time > 2. go to pals-python/schema and add a new element, one at a time, following the existing implementation > 3. go to pals-python/tests/test_schema.py and define a compact test per new element > Repeat this until all elements defined in PALS are implemented in pals-python. And ~20 follow-up prompts.
09c6479 to
c95f8f6
Compare
14555ab to
14d22be
Compare
| # TODO: add ElectricMultipoleParameters in a follow-up RP | ||
| # https://pals-project.readthedocs.io/en/latest/element-parameters.html#electricmultipolep-electric-multipole-parameters |
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 want to keep this PR relatively simple and this property is not.
So let's do this in #38
| # UnionEle | ||
| unionele = pals.UnionEle(name="unionele1", elements=[]) |
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 guess we can leave it here, but what is the purpose of this empty union?
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.
Ah, good point.
We did not talk about this in the PALS standard: do we allow an empty union (e.g., for convenience in lattice manipulations). We can leave it here and allow it for now.
But we should add a test that has actual sub-elements.
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.
Will go in #40
Co-authored-by: Edoardo Zoni <[email protected]>
And more limits on ints and string literal values.
| # Base classes (for testing compatibility) | ||
| BaseElement, | ||
| ThickElement, |
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 will remove this in a follow-up PR: we do not want to expose mixin classes as real element in a concrete BeamLine. I already update the tests accordingly.
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 #40
| passed = True | ||
| try: | ||
| with pytest.raises(ValidationError): | ||
| element.length = element_length | ||
| except ValidationError as e: | ||
| print(e) | ||
| passed = False | ||
| assert not passed |
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.
Great to learn that this can be done in two lines - didn't know that.
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.
Yep! :) I use this in pyAMReX quite a bit to ensure we do not segfault but can recover on issues.
EZoni
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.
I reviewed the commits after my last review and they look good to me. We can do more work on the tests in future PRs, if needed.
Update to include all currently defined elements and parameter classes.
kinds,parameters#34 Clean & Split Tests #36