search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
720 stars 38 forks source link

[REVIEW]: VlaPy: A Python package for Eulerian Vlasov-Poisson-Fokker-Planck Simulations #2182

Closed whedon closed 4 years ago

whedon commented 4 years ago

Submitting author: @joglekara (Archis Joglekar) Repository: https://github.com/joglekara/VlaPy Version: v0.1.0 Editor: @dpsanders Reviewer: @TomGoffrey, @StanczakDominik Archive: 10.5281/zenodo.4026770

Status

status

Status badge code:

HTML: <a href="https://joss.theoj.org/papers/c2b3924d7868d7bd8472c6deb011cfcc"><img src="https://joss.theoj.org/papers/c2b3924d7868d7bd8472c6deb011cfcc/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/c2b3924d7868d7bd8472c6deb011cfcc/status.svg)](https://joss.theoj.org/papers/c2b3924d7868d7bd8472c6deb011cfcc)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@TomGoffrey & @StanczakDominik, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @dpsanders know.

✨ Please try and complete your review in the next two weeks ✨

Review checklist for @TomGoffrey

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @StanczakDominik

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

TomGoffrey commented 4 years ago

This looks great, much improved.

A couple more (I think maybe final!) questions on the article:

  1. You seem to have two near duplicate paragraphs introducing the equations section. For the sake of completeness you should probably define the terms explicitly, although I realise these are standard definitions.
  2. When describing the advection stage, what exactly is the quantity in square brackets you are taking a Fourier transform of? Is the = a typo for +, or should it be =-v...? Should these derivatives be partial derivatives? I think I must be missing something here.
  3. I noticed you have a different sign for the Electric field term in your normalised Vlasov equation compared to Cheng & Knorr (see their equation 1a). Have you used a different normalisation, or is this an error? Looking at Cheng & Knorr equation 2b, it could be your form is correct? Additionally comparing your version of Cheng & Knorr equation 1b, you again seem to have a different sign in the electric field. Perhaps this could all be cleared up with a few extra details when you describe the normalised equations.
  4. Have you assumed gamma = 1? If so this should be stated.
  5. Are you aware of other published results showing the LB operator not conserving momentum for a drifting Maxwellian? If so a reference might put potential users at ease.
joglekara commented 4 years ago

@TomGoffrey , I've made your proposed changes in 1-4

and provided a response to 5 in the thread for the Pull Request here https://github.com/joglekara/VlaPy/pull/45

joglekara commented 4 years ago

@whedon generate pdf from branch origin/paper

whedon commented 4 years ago 
Attempting PDF compilation from custom branch origin/paper. Reticulating splines etc...
whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

TomGoffrey commented 4 years ago

@whedon generate pdf from branch origin/paper

whedon commented 4 years ago 
Attempting PDF compilation from custom branch origin/paper. Reticulating splines etc...
whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

joglekara commented 4 years ago

@TomGoffrey, equation set clarified and a bunch of other edits for clarity and precision. Thanks for this back and forth so far!

joglekara commented 4 years ago

@TomGoffrey Looking forward to your thoughts on the most recent version in origin/paper. Thanks!

joglekara commented 4 years ago

@TomGoffrey @dpsanders looking forward to your thoughts on how to proceed here. Thanks!

TomGoffrey commented 4 years ago

@whedon generate pdf from branch origin/paper

whedon commented 4 years ago 
Attempting PDF compilation from custom branch origin/paper. Reticulating splines etc...
whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

joglekara commented 4 years ago

@whedon generate pdf from branch origin/paper

whedon commented 4 years ago 
Attempting PDF compilation from custom branch origin/paper. Reticulating splines etc...
whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

joglekara commented 4 years ago

@TomGoffrey @dpsanders looking forward to your thoughts on how to proceed here. Thanks!

TomGoffrey commented 4 years ago

Based on that version I only have a couple of comments:

  1. t / \omega isn't a dimensionless time. I presume you mean t \omega. I raised this on the PR for the paper branch a couple of weeks ago but perhaps you didn't notice.
  2. The Vlasov equation (in the limit of zero B-field) for species is df_\alpha / dt = ... + (q_alpha E / malpha) \nabla\alpha f_alpha. (Sorry I don't now how to render math equations on github.) So if you're using e 'the electron charge' to normalise the electric field, I'm not sure how you end up with a negative E term? Am I missing something, or do you mean e ' the elementary charge'?

If you can check these points I'll give the manuscript a final read through over the weekend, but I think we're there.

joglekara commented 4 years ago

@TomGoffrey thanks for the follow up!

  1. Fixed! Thanks for catching this. (I've been working off of https://github.com/joglekara/VlaPy/pull/45 for the past month, and don't see your comment in there. Perhaps it was in a different PR that got merged, so my apologies.)

  2. Ah, thanks for that too. You are very right here, and I got lost in making sure the pesky minus signs were consistent between the (tested) code and the (well-known) equations! I finally traced it back to the fact that the FFT/IFFT operation in NumPy is not symmetric.

joglekara commented 4 years ago

@whedon generate pdf from branch origin/paper

whedon commented 4 years ago 
Attempting PDF compilation from custom branch origin/paper. Reticulating splines etc...
whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

joglekara commented 4 years ago

@TomGoffrey thanks for the follow up!

  1. Fixed! Thanks for catching this. (I've been working off of joglekara/VlaPy#45 for the past month, and don't see your comment in there. Perhaps it was in a different PR that got merged, so my apologies.)
  2. Ah, thanks for that too. You are very right here, and I got lost in making sure the pesky minus signs were consistent between the (tested) code and the (well-known) equations! I finally traced it back to the fact that the FFT/IFFT operation in NumPy is not symmetric.

@TomGoffrey , ^^. Many thanks for your suggestions and comments, as always.

TomGoffrey commented 4 years ago

Sorry, I'm still stuck on the sign issue. You appear to have changed both the sign of the electric field term and the wording of the normalisation. You're now stating (in the description of the normalisation) and e is the fundamental charge - what exactly do you mean by fundamental charge here? I'd read this as being the same as elementary charge i.e. + 1.6e-19 C.

In summary combinations I think are correct:

  1. where e is the charge of an electron ... $ ... + E df / dv$

or

  1. where e is the elementary charge (1.6e-19 C) ... $ ... - E df / dv

Does that make sense, or have I confused myself?

Finally, are the axes on the field solver really 'arbitrary'. Naively I'd assume that x, or \tilde{x} would be more appropriate?

joglekara commented 4 years ago

Hi @TomGoffrey , thanks for this, no reason to apologize! All part of the process in my opinion!

I think that we have to leave the positive sign in $ ... + E df / dv$ with the elementary charge = +1.6e-19. (Changed to elementary because that is more accurate terminology). The reason is because we can't ignore the stationary ion background i.e. \nabla E = \rho_i + \rho_e. My understanding is that the minus sign ends up going in front of rho_e. Does this make sense and remain self-consistent?

As for the field solver, I left those axes as arbitrary as the test is simply recovering integrals of a periodic function over an arbitrary dimension, rather than specifically working with a spatial coordinate. Do you think it's more illustrative / less distracting if I change those to x?

TomGoffrey commented 4 years ago

@joglekara I'm going to discuss the normalisation, in a belated effort to keep this thread clean.

@dpsanders Just to note, whilst I have finished my checklist I'd like to see this point clarified before signing off on it. Hope that's OK.

TomGoffrey commented 4 years ago

@whedon generate pdf from branch origin/paper

whedon commented 4 years ago 
Attempting PDF compilation from custom branch origin/paper. Reticulating splines etc...
whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

joglekara commented 4 years ago

@whedon generate pdf from branch origin/paper

whedon commented 4 years ago 
Attempting PDF compilation from custom branch origin/paper. Reticulating splines etc...
whedon commented 4 years ago

PDF failed to compile for issue #2182 with the following error:

Error producing PDF. ! Package amsmath Error: \tilde allowed only in math mode.

See the amsmath package documentation for explanation. Type H for immediate help. ...

l.396 ...a_D), (\tilde{m} = m / m_e), \tilde{q}

Looks like we failed to compile the PDF

joglekara commented 4 years ago

@whedon generate pdf from branch origin/paper

whedon commented 4 years ago 
Attempting PDF compilation from custom branch origin/paper. Reticulating splines etc...
whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

TomGoffrey commented 4 years ago

@dpsanders Is there anything specific I need to do to mark this review as complete?

dpsanders commented 4 years ago

@TomGoffrey No, thanks, that message is a perfectly good signal. Many thanks for your hard work on this!

dpsanders commented 4 years ago

@whedon generate pdf from branch origin/paper

whedon commented 4 years ago 
Attempting PDF compilation from custom branch origin/paper. Reticulating splines etc...
whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

dpsanders commented 4 years ago

👋 @joglekara: I added a couple of issues in the repo.

joglekara commented 4 years ago

@dpsanders @TomGoffrey Both of your issues had to do with a small update that was necessary for the run file. It's in now.

joglekara commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

joglekara commented 4 years ago

Hi team, Happy Friday (I realize it's already the weekend in Europe...)

On my side, I gave the docs a good cleaning and updating earlier this week.

Any further thoughts on this one?

joglekara commented 4 years ago

Hi @dpsanders , hope all is well.

I see that you're in the midst of a super cool class this fall so just wanted to help out with a friendly bump here ;)

dpsanders commented 4 years ago

👋 @joglekara: Thanks for the bump (and the kind words). Apologies for the absence. I'll do a final check over.

dpsanders commented 4 years ago

@whedon generate pdf