-
Notifications
You must be signed in to change notification settings - Fork 11
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
Overhaul radial distance utility #158
base: main
Are you sure you want to change the base?
Conversation
…uggested by Edgar
I'm beginning to wonder if it might be worth converting this script to an object-oriented version |
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'm really impressed by the application of best practices here @connoramoreno - nice cleanup! I have a few more comments to continue the journey...
def enforce_helical_symmetry(matrix): | ||
"""Ensures that a matrix is helically symmetric according to stellarator | ||
geometry by overwriting certain matrix elements. Assumes regular spacing | ||
between angles defining matrix. |
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.
Doesn't this assume even more than regular spacing? Doesn't it also assume:
- that the matrix represents a full period, and
- that there is a rib occurring at the mid-period
- that the first rib is at the beginning of a period, ie. a place where poloidal symmetry is expected
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.
Good points. I thought I had written it in a way that (2) wouldn't matter but it seems that's not quite true. I think it's reasonable to assume (1) and (3), but I'd like to modify it to accommodate cases when (2) is not true.
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.
Can we update this docstring to clarify the additional assumptions?
Co-authored-by: Paul Wilson <[email protected]>
Co-authored-by: Paul Wilson <[email protected]>
Co-authored-by: Paul Wilson <[email protected]>
Co-authored-by: Paul Wilson <[email protected]>
Co-authored-by: Paul Wilson <[email protected]>
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.
A little final polish...
if (min_tor_ang >= lower_bound or min_tor_ang <= upper_bound) or ( | ||
max_tor_ang >= lower_bound or max_tor_ang <= upper_bound | ||
): | ||
in_toroidal_extent = True | ||
else: | ||
in_toroidal_extent = False |
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 learned about chainging comparisons!
if (min_tor_ang >= lower_bound or min_tor_ang <= upper_bound) or ( | |
max_tor_ang >= lower_bound or max_tor_ang <= upper_bound | |
): | |
in_toroidal_extent = True | |
else: | |
in_toroidal_extent = False | |
in_toroidal_extent = ( (lower_bound <= min_tor_ang <= upper_bound) or | |
(lower_bound <= max_tor_ang <= upper_bound) ) |
def enforce_helical_symmetry(matrix): | ||
"""Ensures that a matrix is helically symmetric according to stellarator | ||
geometry by overwriting certain matrix elements. Assumes regular spacing | ||
between angles defining matrix. |
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.
Can we update this docstring to clarify the additional assumptions?
num_rows = matrix.shape[0] | ||
num_columns = matrix.shape[1] |
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.
num_rows = matrix.shape[0] | |
num_columns = matrix.shape[1] | |
num_rows, num_columns = matrix.shape |
for idx in range(num_rows): | ||
matrix[idx, -1] = matrix[idx, 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.
for idx in range(num_rows): | |
matrix[idx, -1] = matrix[idx, 0] | |
matrix[:,-1] = matrix[:,0] |
for idx, value in enumerate( | ||
flattened_matrix[: int(len(flattened_matrix) / 2)], start=1 | ||
): | ||
flattened_matrix[-idx] = value |
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.
Is this what it's doing?
for idx, value in enumerate( | |
flattened_matrix[: int(len(flattened_matrix) / 2)], start=1 | |
): | |
flattened_matrix[-idx] = value | |
half_period = flattened_matrix[0:int(len(flattened_matrix) / 2)] | |
flattened_matrix = np.concatenate([half_period, np.flip(half_period)]) |
Returns: | ||
smoothed_matrix (2-D iterable of float): smoothed matrix. | ||
""" | ||
previous_iteration = matrix |
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.
previous_iteration = matrix | |
previous_matrix = matrix |
smoothed_matrix = np.minimum( | ||
previous_iteration, | ||
gaussian_filter( | ||
previous_iteration, |
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.
previous_iteration, | |
previous_matrix, |
mode="wrap", | ||
), | ||
) | ||
previous_iteration = smoothed_matrix |
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.
previous_iteration = smoothed_matrix | |
previous_matrix = smoothed_matrix |
Overhauls the radial distance utility. Changes include: