Use list comprehension for shift the theta coordinate from 0 to 2pi #164
Conversation
parastell/nwl_utils.py
Outdated
| theta_coords = [ | ||
| (theta_coord + 2 * np.pi) % 2 * np.pi for theta_coord in theta_coords | ||
| ] |
There was a problem hiding this comment.
Might be better to change theta_coords to a NumPy array
There was a problem hiding this comment.
Moving to arrays rather than lists would be somewhat large rework, as we make use of append to deal with the variable length iterables that come from the multithreading approach, I'd prefer to keep this PR as a minimal change to get things into a working state.
Regardless, I realized we can just put this coordinate shift in the function where we calculate theta, which I think is a bit more elegant.
There was a problem hiding this comment.
The whole nwl_utils could possibly be reworked similar to what Connor has done for radial_distance_utils, but I think that would be better for a different PR.
There was a problem hiding this comment.
I'm happy with this change if @gonuke approves. See my comment about documentation.
| @@ -145,7 +145,7 @@ def find_coords(data): | |||
| bounds=[(-np.pi, np.pi)], | |||
| args=(vmec, wall_s, coords[0], coords[1]), | |||
| ) | |||
| thetas.append(theta.x[0]) | |||
| thetas.append((theta.x[0] + 2 * np.pi) % (2 * np.pi)) | |||
There was a problem hiding this comment.
For our future benefit, it's best to add a comment here explaining (briefly) this conversion
There was a problem hiding this comment.
LGTM! I'll wait to see what Paul thinks before merging. Thanks @Edgar-21
I experienced an error with nwl_utils.py as currently written, since theta_coords is a list. This approach, or changing theta_coords to a numpy array will alleviate that error.
Fixed a missing comma in invessel_build.py