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 periodic masking option to Aperture. #739
Add periodic masking option to Aperture. #739
Changes from 7 commits
1948629
fea841c
8b1ed60
3323f51
110e07b
1412a5a
2b7dfbf
a5714ec
099e19e
5c67be0
520b5c9
90497ed
c05e2c4
914b0f7
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.
Do we need that line?
I see, this variable is being used in line 60.
I wonder why this number is hard-coded instead of directly defined as len(initial), as it doesn't matter at which initial number of particles this test is being done.
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 is only to check that the number of particles generated, and the number of particles remaining after the lattice (including those lost) both coincide with the number specified in the input file. We have this check in all of our CI tests.
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.
For the example that users are looking at, it would be nice to add another drift and monitor after the pepperpot to complement the setup of an actual emittance measurement and plot the outcome on the last monitor.
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.
Agreed. For automated testing, the existing test is good because it is simple and checks the functionality implemented. I propose that we keep it as-is for this PR, and we can add a follow-up example that includes a full pepperpot emittance reconstruction. We could rename the existing test
aperture_periodic
to avoid confusion, and name the follow-up testpepperpot_measurement
or something similar.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. Maybe it is worth adding another drift and monitor to represent an actual pepperpot emittance measurement setup.
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.
Absolutely! See the above suggestion.