Closes #19: Adding capability for charged particle motion in electric and magnetic fields

[gsgall]

• VelocityUpdater was refactored to ParticleStepper to be more descriptive
• The updateVelocity method was changed to setupStep since it does more than just update the particle velocity

(Closes #19)

[moosebuild]

Job Documentation on 1ca808c wanted to post the following:

View the site here

This comment will be updated on new commits.

[moosebuild]

Job Coverage on 1ca808c wanted to post the following:

Coverage

	042c1a	#20 1ca808
	Total	Total	+/-	New
Rate	91.53%	91.90%	+0.38%	94.00%
Hits	162	227	+65	94
Misses	15	20	+5	6

Diff coverage report

Full coverage report

This comment will be updated on new commits.

[simopier]

Thank you for this contribution!

A partial first round of reviews (I did not make it to the tests yet).
It looks like a lot of comments, but these are generally minor since you submitted a very good base.

EDIT: by the way @gsgall , Casey taught me something I'm now sharing every chance I get: if you go to the file changes, you can submit commits in batches from suggestions.

Let me know if you have any questions.

[simopier]

I just looked at the test coverage, @gsgall:

• you'll want to add tests activating the error from if (******.size() != 3) check.
• Also, it looks like the unused methods in ParticleStepperBase are indeed unused. Do they need to stay there? (@loganharbour)
• it also look like the following lines are not hit:

const std::vector<std::shared_ptr<Ray>> &
PICStudyBase::getBankedRays() const
{
  return _banked_rays;
}

What are they for?

[loganharbour]

All pretty minor here.

[loganharbour]

it also look like the following lines are not hit:

Heh @simopier noticed without even looking at the coverage 😬

[gsgall]

it also look like the following lines are not hit:

Heh @simopier noticed without even looking at the coverage 😬

Sorry about that. I was using that method when I had written a test that output rayData into csv with a custom post processor object. I forgot to remove it when I transitioned the tests to be exodiffs because the post processor test was not parallel consistent.

[gsgall]

@loganharbour @simopier This should be good to go.

[loganharbour]

My last comment. Content with everything else

[simopier]

Thank you for these improvements, @gsgall. I have a few more minor comments and two suggestions:

• could you add a test case for the constant electric field case?
• I encourage you to add a reference to your submitted PBNC paper in the relevant documentation pages. This will improve its visibility. You can lark it as submitted, and we'll update this when it gets published.

[gsgall]

Thank you for these improvements, @gsgall. I have a few more minor comments and two suggestions:

• could you add a test case for the constant electric field case?
• I encourage you to add a reference to your submitted PBNC paper in the relevant documentation pages. This will improve its visibility. You can lark it as submitted, and we'll update this when it gets published.

I have added these test cases for the BorisStepper I also left a comment about them on your review requests where you ask for them as well.

Additionally, the paper reference has been added to the BorisStepper documentation page.

[simopier]

Thanks @gsgall, this looks great to me.

@loganharbour, could we add the json file capabilities you mentioned to make the gold file more human readable? Maybe in a follow up PR if it isn't ready to go in yet.

[loganharbour]

@loganharbour, could we add the json file capabilities you mentioned to make the gold file more human readable? Maybe in a follow up PR if it isn't ready to go in yet.

Yeah, this'll be a future effort. It's on a list :)