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

Parallelize nwl #122

Merged
merged 9 commits into from
Jul 16, 2024
Merged

Parallelize nwl #122

merged 9 commits into from
Jul 16, 2024

Conversation

Edgar-21
Copy link
Contributor

addresses #120

diff = np.linalg.norm(pt - fw_pt)

return diff


def find_coords(vmec, wall_s, phi, pt):
def find_coords(data):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this function takes a tuple so it plays nicely with concurrent.futures.ProcessPoolExecutor

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

We'll have to get everyone on the same PEP8 autoformatting to stop thrashing between line length and quotation styles :)

@connoramoreno
Copy link
Collaborator

I have autopep8 installed, not sure if others would prefer something else.

@Edgar-21
Copy link
Contributor Author

Edgar-21 commented Jun 27, 2024

I changed to black since autopep8 wasn't being aggressive enough for the way I write code apparently 🙃. It does a better job of enforcing quotations and whitespace (and stops me from putting closing brackets on the line I'm not supposed to). It did require that I manually specify an 80 character line limit, I think it's default is 88.

@Edgar-21
Copy link
Contributor Author

Edgar-21 commented Jul 3, 2024

To test that this parallelization is working as expected I ran 10000 particles from a surface source using the main branch, and wrote the resulting matrix to file, then did the same but with this branch, using 9 threads. I then loaded the files to compare the values and found that they were identical. Not a very formal test, but I could make a PR adding testing for nwl_utils then once that's good to go this PR would be tested against that?

@Edgar-21 Edgar-21 marked this pull request as ready for review July 3, 2024 15:03
Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

Thanks for sticking with this @Edgar-21

parastell/nwl_utils.py Outdated Show resolved Hide resolved
parastell/nwl_utils.py Outdated Show resolved Hide resolved
parastell/nwl_utils.py Outdated Show resolved Hide resolved
Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

LGTM - thanks @Edgar-21

@gonuke gonuke merged commit fe943e2 into main Jul 16, 2024
3 checks passed
@Edgar-21 Edgar-21 deleted the parallelize_nwl branch August 5, 2024 13:31
connoramoreno pushed a commit that referenced this pull request Sep 10, 2024
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