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

Spin tracking #672

Merged
merged 25 commits into from
Apr 26, 2024
Merged

Spin tracking #672

merged 25 commits into from
Apr 26, 2024

Conversation

pots007
Copy link
Contributor

@pots007 pots007 commented Dec 1, 2023

This PR implements spin tracking in FBPIC.

The API is (hopefully) straightforward: the Particle class now has a method activate_spin_tracking, which turns on spin tracking by creating a new instances on SpinTracker. All necessary arrays are stored inside this object. The user can set the average polarisation and have the spins be generated randomly or set to a fixed value.

@MaxThevenet MaxThevenet mentioned this pull request Dec 7, 2023
67 tasks
Copy link
Member

@RemiLehe RemiLehe left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this impressive amount of work!! I love that the code is high-quality, has several automated tests, and is well documented! 🚀 ✨

I added a number of suggestions and comments. Most of them are cosmetic. Some of the more significant comments are the following:

  • One of the automated tests did not pass on GPU when I tried. Would you have time to fix it?
  • The function that initialized the spin in the case 'random' with a non-zero mean seemed a bit obscure to me. I suggested another function (see corresponding Python code in the inline comments).
  • Did anyone test this in the boosted frame? Also: do you have a reference on how spins transform under a Lorentz transform? If yes, we could modify the initialization of the spin, in the case of a boosted-frame simulation, to make it consistent with the fact that the spins are initialized in a different Lorentz frame.

tests/test_spin_ionization.py Outdated Show resolved Hide resolved
tests/test_spin_ionization.py Outdated Show resolved Hide resolved
tests/test_spin_ionization.py Outdated Show resolved Hide resolved
tests/test_spin_ionization.py Outdated Show resolved Hide resolved
tests/test_spin_ionization.py Outdated Show resolved Hide resolved

return sx, sy, sz

def generate_ionized_spins_gpu(self, Ntot):
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a brief docstring to this function?
In particular, it might good to say this generates the spins according to the specifications of self.spin_distr, self.sx_m, self.sy_m, self.sz_m.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out this was an oopsie: I implemented the random_point_sphere_gpu method directly in the ionizer. The code is much cleaner now, thanks for pointing this out.

fbpic/particles/spin/spin_tracker.py Outdated Show resolved Hide resolved
fbpic/particles/spin/inline_functions.py Outdated Show resolved Hide resolved
fbpic/particles/spin/inline_functions.py Outdated Show resolved Hide resolved
fbpic/particles/spin/inline_functions.py Show resolved Hide resolved
@pots007
Copy link
Contributor Author

pots007 commented Apr 25, 2024

Hey @RemiLehe , thanks a lot for the review and sorry it took me so long to get the changes done! I think I've addressed most of your points:

  • the error I cannot reproduce. Could you let me know which fbpic file is the source of the error?
  • the random spin generation is now based on your suggestion - that is much, much better
  • Updated lots of docstrings
  • Removed some weird circular function calls
  • removed storing species as you suggested
  • improved spin_ionization test
  • etc

I have not yet done the boosted frame tests, I need a bit more time to think about that.

If you had time to check over the code again, it'd be great!

@RemiLehe
Copy link
Member

RemiLehe commented Apr 25, 2024

Great, thanks implementing these changes!
It looks like we both tried to fix the restart test at the same time ; in the end I pushed a slightly different fix.

Now all the tests are working on GPU on my side too, thanks for the fix!

I will take one last look soon and will then merge.

@RemiLehe RemiLehe enabled auto-merge April 25, 2024 23:26
@RemiLehe RemiLehe merged commit b8836f0 into fbpic:dev Apr 26, 2024
1 check passed
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.

None yet

2 participants