-
Notifications
You must be signed in to change notification settings - Fork 9
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
Beam stats #129
Beam stats #129
Conversation
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.
Looks good overall, but I have suggestions on the efficiency for the utils code.
sirepo_bluesky/srw_handler.py
Outdated
"units": units, | ||
"units": "units", |
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.
We have a variable returned from srw_io.file_load(filename)
called units
. Was the overwrite intentional?
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.
oops, I'll fix this
sirepo_bluesky/utils/__init__.py
Outdated
mean_x = np.sum(X * image) / np.sum(image) | ||
mean_y = np.sum(Y * image) / np.sum(image) | ||
|
||
sigma_x = np.sqrt(np.sum((X - mean_x) ** 2 * image) / np.sum(image)) | ||
sigma_y = np.sqrt(np.sum((Y - mean_y) ** 2 * image) / np.sum(image)) |
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.
np.sum(image)
is done 4 times. Please calculate it once and reuse it.
def get_beam_stats(image, x_extent, y_extent): | ||
n_y, n_x = image.shape | ||
|
||
if image.sum() > 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.
image.sum()
is used here and in return. Let's calculate it once and use in all 6 places.
sirepo_bluesky/utils/__init__.py
Outdated
"fwhm_x": 2 * np.sqrt(2 * np.log(2)) * sigma_x, | ||
"fwhm_y": 2 * np.sqrt(2 * np.log(2)) * sigma_y, |
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.
The factor can be calculated once and reused. Please update.
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.
Looks good to me.
@AbbyGi, @BriannaRomasky, could you also please have a look?
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 a question, are 'x' and 'y' the centroid position?
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 think so.
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.
Looks good!
Added a lightweight method that computes the beam position and FWHM before it's turned into a document. Now the beam stats show up in the callback!