Skip to content
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

bugfixes from run #213

Merged
merged 3 commits into from
Nov 27, 2024
Merged

bugfixes from run #213

merged 3 commits into from
Nov 27, 2024

Conversation

roussel-ryan
Copy link
Collaborator

Fixes the following bugs:

  • image reshaping incorrect
  • improper validation of rmats
  • incorrect attribute reference for design_twiss
  • uses dict output from compute_emit_bmag

@roussel-ryan roussel-ryan requested a review from nneveu November 26, 2024 15:57
@roussel-ryan
Copy link
Collaborator Author

@dylanmkennedy can you verify the correct shape for rmat validation?

results = {
"emittance": emittance,
"BMAG": bmag,
results = results | {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My $.02.

results.update() would be more efficient than |. Perhaps |= would be just as efficient.

Another consideration, the | operator only works with Python 3.9+. This would be an unnecessary breaking change. 3.8 is just at end of life. Yes, people should update, but environments are not always up to date, e.g. (I believe) RHEL7 is still used actively at SLAC.

In general, I find overloaded operators harder to read. I had to double-take this line in a cursory glance. Named functions are easy for humans to read, especially humans unfamiliar with the latest features, which busy physicists usually are. Also, using the "latest and greatest" (unless it is necessary) does not improve readability.

@@ -73,7 +73,7 @@ def image(self) -> np.ndarray:
the camera associated with this screen
"""
return self.controls_information.PVs.image.get(as_numpy=True).reshape(
self.n_rows, self.n_columns
self.n_columns, self.n_rows
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did this mistake get caught? I think I'm remembering this might not be the case for all cameras. Will approve for now and check with Courtney.

@nneveu nneveu merged commit d2c9094 into main Nov 27, 2024
3 checks passed
williamColocho pushed a commit to williamColocho/lcls-tools that referenced this pull request Dec 6, 2024
* bugfixes from run

* change dict update statement

* Update emittance_measurement.py
@roussel-ryan roussel-ryan deleted the bugfixes branch February 18, 2025 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants