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

Implement magnetic field propagator #221

Merged
merged 30 commits into from May 5, 2021

Conversation

whokion
Copy link
Contributor

@whokion whokion commented Apr 19, 2021

The first version of FieldPropagator and associated tests:

  • Added FieldTrackView
  • Added FieldPropagator
  • Added state accessors of GeoTrackView to support FieldTrackView
  • Added FieldPropagator.test

@whokion whokion requested review from sethrj and pcanal April 19, 2021 19:11
@sethrj sethrj added the field Magnetic field and propagation label Apr 20, 2021
@sethrj sethrj added this to In progress in v0.1.0 via automation Apr 20, 2021
@sethrj sethrj moved this from In progress to Review in progress in v0.1.0 Apr 20, 2021
@sethrj sethrj added this to the Y1Q4 Goals milestone Apr 20, 2021
Copy link
Member

@sethrj sethrj left a comment

Choose a reason for hiding this comment

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

I might not be able to resume the review today, so here are a couple of comments. I think we should try to encapsulate all the navstate-related aspects of the FieldState into the GeoTrackView itself, but that's just my first instinct rather than something I've really thought through about what parts (e.g. step and safety) belong where.

src/field/FieldPropagator.i.hh Outdated Show resolved Hide resolved
src/field/FieldPropagator.i.hh Outdated Show resolved Hide resolved
src/field/FieldPropagator.hh Outdated Show resolved Hide resolved
src/field/FieldPropagator.i.hh Outdated Show resolved Hide resolved
src/field/FieldPropagator.i.hh Outdated Show resolved Hide resolved
src/field/FieldTrackView.hh Outdated Show resolved Hide resolved
src/field/FieldUtils.i.hh Outdated Show resolved Hide resolved
src/field/FieldTrackView.hh Outdated Show resolved Hide resolved
@whokion
Copy link
Contributor Author

whokion commented Apr 20, 2021

@sethrj Regarding to navstate-related interfaces, I am totally open to move things to GeoTrackView, which potentially remove unnecessary public accessors of vgstates which I introduced.

Copy link
Member

@sethrj sethrj left a comment

Choose a reason for hiding this comment

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

OK, I think the questions about the persistent state (safety distance) and FieldPropagator will help shape how we can move some of the pieces into the GeoTrackView.

src/field/FieldPropagator.hh Outdated Show resolved Hide resolved
src/field/FieldTrackView.i.hh Outdated Show resolved Hide resolved
src/field/FieldTrackView.i.hh Outdated Show resolved Hide resolved
src/field/FieldTrackView.hh Outdated Show resolved Hide resolved
src/field/FieldPropagator.hh Outdated Show resolved Hide resolved
@whokion
Copy link
Contributor Author

whokion commented Apr 27, 2021

@sethrj I guess that I resolved most of outstanding issues for this MR besides those related to GeoTrackView and utility interfaces/functions using the vecgeom navigator, which we may be addressed in a separate MR. Can review the change
and consider to merge this MR if there are no obvious things to changes at this moment. Thanks.

@sethrj
Copy link
Member

sethrj commented Apr 28, 2021

@whokion Thanks, sorry I didn't have a chance to merge this or review it in more detail. Can you, me, @pcanal, @amandalund, maybe @tmdelellis interested meet today at 2 pm eastern/3 central to discuss the interface? I'd like to get that in place before resuming the review.

@whokion
Copy link
Contributor Author

whokion commented Apr 28, 2021

@sethrj Yes, I am available anytime this afternoon.

@whokion
Copy link
Contributor Author

whokion commented Apr 30, 2021

@sethrj I would like to close this MR (delete the branch) and open a new one as this branch adding new files has been significant changed from the initial version (i.e, removed FieldTrackView and un-needed public (vgstate) accessors of GeoTrackView which were introduced for the defunct FieldTrackView, and changed many interfaces with using GeoTracView instead of FieldTrackView and etc.) - you may take a quick look at the current branch, which is very close to an upcoming new one except a coupe of more clean ups (such as returning pos and dir instead of OdeState which should be agnostic to GeoTrackView or the higher level transport interface or class. One more question is whether FieldPropagator can modify GeoTrackView data (i.e, pos and dir by available stters) directly. In the real work flow, I think it should be the case, but we may also send an intermediate result of the field propagation to the high level transportation manager which can update things accordingly in one central place. As shortly discussed last Wednesday, we agreed to go with the latter (which is reflect in the current implementation), but the first option will also reduce unnecessary data copy. Either is okay for me now, but just want to know your preference. One more note is that the current field propagator updates the final vgstate inside the class anyway as the final direction from the last sub-step before the boundary is used for the final direction for updating the vgstate, which is different from the final (momentum) direction by the stepper. Anyway, please let me know whether I can delete this one and create a new one. Thanks.

Soon Yung Jun added 2 commits May 4, 2021 11:19
@whokion
Copy link
Contributor Author

whokion commented May 4, 2021

@sethrj This MR is now ready to be reviewed. Thanks.

Copy link
Member

@sethrj sethrj left a comment

Choose a reason for hiding this comment

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

I'm satisfied. Thanks for the iterations!

src/field/FieldDriver.hh Outdated Show resolved Hide resolved
src/field/FieldPropagator.hh Outdated Show resolved Hide resolved
test/CMakeLists.txt Outdated Show resolved Hide resolved
@sethrj sethrj merged commit d02ea08 into celeritas-project:master May 5, 2021
v0.1.0 automation moved this from Review in progress to Done May 5, 2021
@sethrj
Copy link
Member

sethrj commented May 5, 2021

Thanks @whokion!

@whokion
Copy link
Contributor Author

whokion commented May 5, 2021

@sethrj Thanks a lot for all reviews and the merge. The next plan is to add/support 1) a non-uniform magnetic field case, 2) a small angle approximation for detecting the boundary intersection. Of course, I will closely follow up related interfaces to geometry/transport processes as well as further improvement/optimization of existing field codes. If you have a different priority and plan, please let's discuss at our usual meetings.

@whokion whokion deleted the field-propagator branch May 5, 2021 17:22
@sethrj sethrj mentioned this pull request Sep 24, 2021
4 tasks
@sethrj sethrj changed the title Magnetic Field Propagator (#49) Magnetic Field Propagator Nov 14, 2023
@sethrj sethrj changed the title Magnetic Field Propagator Implement magnetic field oropagator Nov 14, 2023
@sethrj sethrj changed the title Implement magnetic field oropagator Implement magnetic field propagator Nov 14, 2023
@sethrj sethrj added the enhancement New feature or request label Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request field Magnetic field and propagation
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants