celeritas-project / celeritas

Celeritas is a new Monte Carlo transport code designed to accelerate scientific discovery in high energy physics by improving detector simulation throughput and energy efficiency using GPUs.
https://celeritas-project.github.io/celeritas/user/index.html
Other
62 stars 32 forks source link

Add neutron inelastic process #1150

Closed whokion closed 5 months ago

whokion commented 5 months ago

Add neutron inelastic process (minimal skeleton)

Description

This MR includes minimal skeletons of neutron inelastic processes for an initial development.

whokion commented 5 months ago

This is a set of classes for neutron capture and inelastic processes and models (without any final state sampling). It may require to refactor many codes later as there are somewhat redundant codes in the current structure, but it would be good to keep them separate until all sub-components are implemented and tested - later, we may consolidate codes into a neutron general process, which may be more compact and efficient. Anyway, these codes will serve as a ground for implementing and testing inreractor codes along the way.

whokion commented 5 months ago

@sethrj Sorry for this big MR! Even though this MR can be separated by two, it actually follows a fairly parallel structure for capture and inelastic (as this set of classes are only loading cross section data and building the pipeline from process to interactor for capture and inelastic). Since there are no much variations between capture and inelastic without detail features for each interactor, I hope that they should be straightforward to review in parallel). If it is too much load to review, please take your time. I promise that this is the last big MRs for neutrons. Otherwise, I will break it by two. If the size of cross section test data is too big to keep under the test area , I can prune them for minimal tests.

sethrj commented 5 months ago

Please @whokion , you know we have a policy to keep pull requests as a small as possible to make them more reviewable. The fact that the two new processes are similar is another good reason to keep them separate. It's very hard to cross reference the multiple files within a single pull request interface.

I would recommend going through your pull request and simply deleting all the files/lines that aren't (e.g.) capture. Make sure that it builds, commit the changes, run git checkout -b neutron-inelastic and git revert HEAD. That gives you a new starting point for the follow-on pull request.

whokion commented 5 months ago

No problem. Will break by two. Sorry about not knowing the policy.

sethrj commented 5 months ago

Huh, I guess it wasn't officially codified, sorry for that omission. I'll update the contribution guide. It's definitely a best practice in general to have pull requests be as possible while still "substantive and self-contained" . See for example https://github.com/celeritas-project/celeritas/pull/1119 where I broke up a 1400 line PR into several different ones for Elliott.

whokion commented 5 months ago

I would recommend going through your pull request and simply deleting all the files/lines that aren't (e.g.) capture. Make sure that it builds, commit the changes, run git checkout -b neutron-inelastic and git revert HEAD. That gives you a new starting point for the follow-on pull request.

@sethrj I removed all codes related to the capture and am ready to push the update to the remote (neutron-processes). Just curious, why do I need git checkout -b neutron-inelastic and git revert HEAD? So, do I need to create another branch (neutron-inelastic) and push it to the remote? Thanks.

sethrj commented 5 months ago

@whokion the recommendation I made was how to create a second "draft" branch that restores the removed code. So if you've deleted all the capture code in a single commit, you can create a new branch that restores all that code:

$ git commit # after removing all the capture code
$ git push # makes this PR (#1150) only about inelastic
$ git checkout -b neutron-capture # create a new branch for a future merge request
$ git revert HEAD # restore all the neutron capture code
$ git push -u # push so that you can create a second "draft" PR
$ git checkout neutron-processes # so that you're ready to work on PR 1150 again
sethrj commented 5 months ago

@whokion please merge and push, rather than force-pushing. It looks like you also have pushed some conflict markers.

whokion commented 5 months ago

@sethrj Sorry about that. Somehow I also messed up my local branch while I rebased it to the develop and tried to resolve conflicts. As there were too many update since this was opened, I would like to close this MR and reopen it later.

sethrj commented 5 months ago

OK. If you need a hand with git reflog, I can help restore your history (as long as it was git committed). It looks like d51d342 was the last commit you had before overwriting.