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

Add neutron elastic scattering physics #1120

Merged
merged 40 commits into from
Mar 7, 2024

Conversation

whokion
Copy link
Contributor

@whokion whokion commented Feb 22, 2024

Add neutron elastic scattering physics

  • Implement neutron-nucleus elastic scattering process, model, interactor and associated classes [enhancement, physics]
  • Add a test for neutron-nucleus elastic scattering model and interactor
  • Update import, process build, etc. for the newly added neutron elastic process and model

Description

The MR includes the first implementation of the neutron physics (neutron elastic scattering) based on the Geant4's CHIPS model as a part of add neutron physics in the medium-high energy range (up to 20GeV). The implementation is a minimal
without generating secondary ions, but features should be good enough for testing neutron transport in typical HEP user cases. A minimal verification (cross section, scattered energy spectrum of neutrons and direction) has been done with the default Geant4 physics list, but more rigorous validations and verification are required once a test transport application is added.

@whokion whokion added enhancement New feature or request physics Particles, processes, and stepping algorithms labels Feb 22, 2024
@whokion
Copy link
Contributor Author

whokion commented Feb 22, 2024

Sorry for a big set of commits! Please take your time as this MR should be totally decoupled from other tasks. 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.

A few initial comments... this looks great overall!

src/celeritas/io/NeutronXsReader.cc Outdated Show resolved Hide resolved
src/celeritas/io/NeutronXsReader.cc Outdated Show resolved Hide resolved
src/celeritas/io/NeutronXsReader.cc Outdated Show resolved Hide resolved
src/celeritas/io/NeutronXsReader.cc Show resolved Hide resolved
src/celeritas/neutron/data/NeutronElasticData.hh Outdated Show resolved Hide resolved
Copy link
Contributor

@amandalund amandalund left a comment

Choose a reason for hiding this comment

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

This looks great @whokion! Still working through it, but here are a few initial comments.

src/celeritas/io/NeutronXsReader.cc Outdated Show resolved Hide resolved
/*!
* Calculates the macroscopic cross section.
*/
class NeutronElasticMacroXsCalculator
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is essentially identical to the Livermore macro xs calculator, I wonder if there's a way we could use the same calculator for both and reduce the duplicate code...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea as I will also add a similar set of calculators for NeutronInelastic. I think that a templated class, something like,

template<class MicroXsT, typename MacroXsUnitsT>
class MacroXsCalculator

may be used as a common method. As a quick look, this requires changes in the associated MicroXsCalculator (argument for constructor and operator) and a creation of a specific MicroXsCalculator before hand which imposes an explicit rule for MacroXsCalculator. Please let me know if you have the better idea or if a potential consolidation may be a separated MR.

Copy link
Member

Choose a reason for hiding this comment

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

I like that idea. I think it would be better to create a new PR that refactors LivermorePEMacroXsCalculator to something like:

template<class F>
class MacroXsCalculator
{
    using ParamsRef = typename F::ParamsRef;
    using Energy = typename F::Energy;
    using XsUnits = units::Native;

    MacroXsCalculator(ParamsRef const&, MaterialView const& material);
    real_type operator()(Energy) const;
};

I think also LivermorePEMicroXsCalculator should be returning either a units::Barn quantity or a native (len^2) unit.

Comment on lines 422 to 423
auto calc_xs = EPlusGGMacroXsCalculator(
params_.hardwired.eplusgg_data, material);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
auto calc_xs = EPlusGGMacroXsCalculator(
params_.hardwired.eplusgg_data, material);
auto calc_xs = NeutronElasticMacroXsCalculator(
params_.hardwired.chips_data, material);

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's time to add kernels to replace these hard coded xs bits?

src/celeritas/neutron/data/NeutronElasticData.hh Outdated Show resolved Hide resolved
src/celeritas/neutron/data/NeutronElasticData.hh Outdated Show resolved Hide resolved
src/celeritas/neutron/data/NeutronElasticData.hh Outdated Show resolved Hide resolved
src/celeritas/neutron/data/NeutronElasticData.hh Outdated Show resolved Hide resolved
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.

@whokion I'm pausing my review because I think there's a lot of naming updates that need to be done, and it seems like you might have addressed some commits but not pushed.

infile >> result.x[i] >> input_xs.value();
result.y[i] = native_value_from(input_xs);
}
infile.close();
Copy link
Member

Choose a reason for hiding this comment

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

Forgot to add that close is redundant with the destructor (we're at the end of the block so it's going out of scope. The only reason to call it independently is to be able to check bool(infile) afterward to make sure it closed correctly, which frankly doesn't matter.

Suggested change
infile.close();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed!

*/
struct NeutronElasticIds
{
ActionId action;
Copy link
Member

Choose a reason for hiding this comment

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

The action IDs no longer need to be stored in device data like this (they could be instead stored as member data on the Model class itself). The only place it's used is in the Model where the action ID is returned (and indirectly where make_action_track_executor is used).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As all other models/data have this ActionId consistently, It may be better to refactor all of them together with a separate MR (?).

Comment on lines 24 to 28
//!@{
//! \name Type aliases
using Real3 = Array<real_type, 3>;
//!@}

Copy link
Member

Choose a reason for hiding this comment

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

You should be able to remove this and just use the typedef in geocel/Types.hh.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

inline CELER_FUNCTION void boost(Real3 const& v, FourVector* p)
{
const real_type v2 = dot_product(v, v);
CELER_ENSURE(v2 < real_type{1});
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
CELER_ENSURE(v2 < real_type{1});
CELER_EXPECT(v2 < real_type{1});

also vsq or v_sq ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed as suggested.

boost(boost_vector(lv), &nlv1);

result.direction
= make_unit_vector(rotate(make_unit_vector(nlv1.mom), inc_direction_));
Copy link
Member

Choose a reason for hiding this comment

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

The outer call to make unit vector is unnecessary since rotating a unit vector will result in a unit vector

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. Thanks - forget to remove the outer one when I added the inner one. Done.


// Sample the momentum transfer
template<class Engine>
inline CELER_FUNCTION real_type operator()(Engine& rng);
Copy link
Member

Choose a reason for hiding this comment

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

Is the result MevMomentum? It should definitely be a Quantity then. I think you can also use quantity to handle the transfer to and from GeV internally...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually the result is $({\rm MevMomentum})^{2}$.

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.

More comments now that I've had a chance to dig down into the guts of the momentum transfer sampler 😬

* \note This performs the same sampling routine as in Geant4's
* G4ChipsElasticModel and G4ChipsNeutronElasticXS where the differential
* cross section for quark-exchange and the amplitude of the scattering are
* parameterized in terms of the neutron momentum (GeV/c) in the lab frame.
Copy link
Member

Choose a reason for hiding this comment

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

How hard would it be to preprocess the data so that its units are consistent with the rest of the code? It seems a little odd to divide by 1000 when entering and then multiply by 1000 when leaving. If we don't have to do the extra division and multiplication, we can also make stronger guarantees about the resulting value and skip the extra clamping.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally agree! Not sure why the parameterization was done in GeV. Nevertheless, I am not sure whether it is easy to re-parameterize those coefficients by MeV in order to remove this extra conversion. For the moment, I would like to keep it as it is until all validations are done, then we may refactor it later.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it looks like there are a lot of odd exponents and factors...

real_type i_3 = r_3 * par_q_sq_.expnt[2];
real_type i_4 = r_4 * par_q_sq_.expnt[3];
real_type i_5 = i_1 + i_2;
real_type i_6 = i_3 + i_5;
Copy link
Member

Choose a reason for hiding this comment

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

On GPU it might be better to not precalculate these: we should do a test compile of this kernel and see if the register pressure here is high. (That can be a follow-on "optimization" check, just something to keep in mind.)

test/celeritas/neutron/NeutronElastic.test.cc Outdated Show resolved Hide resolved
test/celeritas/neutron/NeutronElastic.test.cc Outdated Show resolved Hide resolved
test/celeritas/neutron/NeutronElastic.test.cc Show resolved Hide resolved
@whokion
Copy link
Contributor Author

whokion commented Mar 6, 2024

@sethrj Trying to understand those failures 1) Are tests with float using different seeds or are those failures due to something else? 2) in the last test (clhep, ... ubuntu-cuda), values are off by a factor 10 which I do not understand. Have any idea? Thanks.

@sethrj
Copy link
Member

sethrj commented Mar 6, 2024

@whokion the single-precision failures are because the RNG stream changes and everything is super sensitive. if you look at the test/celeritas/CMakeLists.txt you'll see that most of the EM tests have ${_needs_double} as part of the test call.. that'll disable the test for double precision.

For the failing clhep test, you need to do something like

        macro_xs.push_back(
            native_value_to<units::InvCmXs>(calc_macro_xs(MevEnergy{e})).value());

@whokion
Copy link
Contributor Author

whokion commented Mar 6, 2024

@sethrj, @amandalund Thank you for all your review comments! I hope that most of suggestions are reflected now and the MR may be good enough to be merged - I will continue to improve codes as I find more documentation related to the model and do more tests along the way. Please let me know any major things that I missed to address. Thanks again!

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 still feel icky about "here's some code that reproduces geant4 but we have no idea what it means, what it does, or where it's from", but I guess that's a good enough starting point for reference and comparison?

@whokion
Copy link
Contributor Author

whokion commented Mar 6, 2024

I still feel icky about "here's some code that reproduces geant4 but we have no idea what it means, what it does, or where it's from", but I guess that's a good enough starting point for reference and comparison?

I fully agree! On the other hand, we actually know what each component does and how things are related even though detail parameterizations (usually with respect to a certain data set, which sometime even have uncertainties or incompleteness) are not known or fully justified as the nature of NP would not reveal underlying physics cleanly. Anyway, the best standard candle may be physics validation with respect to data besides Geant4 and other MCs results. I will search more documentation on CHIPS or try to contact Kossov, if reachable, how the chips parameterizations were done and put more documentation into the code. After all, details may not be sometimes digestible, but still good to have solid background information on them. Otherwise, we should write a new simulation model based on the celeriton theory.

Copy link
Contributor

@amandalund amandalund left a comment

Choose a reason for hiding this comment

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

Great work @whokion, thanks for our first neutron model! I agree there are still some parts that could be clarified/improved, but it makes sense to wait until after we've done some initial validation.

Items<real_type> reals;
ElementItems<GenericGridData> micro_xs;

//! A-dependent coefficients for the momentum transfer) of the CHIPS model
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
//! A-dependent coefficients for the momentum transfer) of the CHIPS model
//! A-dependent coefficients for the momentum transfer of the CHIPS model

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

CELER_EXPECT(!inp.empty());
particle_params_ = std::make_shared<ParticleParams>(std::move(inp));
ps_ = StateStore<ParticleStateData>(particle_params_->host_ref(), 1);
// cutoff_params_ = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// cutoff_params_ = {};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Comment on lines +78 to +94
std::vector<real_type> const expected_micro_xs = {7.77548e-24,
7.54911e-24,
5.5795e-24,
1.52568e-23,
6.55421e-24,
3.32152e-24,
1.89914e-24,
1.16444e-24,
4.78256e-25,
5.04635e-25};

real_type energy = 1e-5;
real_type const factor = 1e+1;
for (auto i : range(expected_micro_xs.size()))
{
XsCalculator calc_micro_xs(shared, MevEnergy{energy});
EXPECT_SOFT_EQ(calc_micro_xs(el_id), expected_micro_xs[i]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since these cross sections are so small, I think even EXPECT_SOFT_EQ(0, calc_micro_xs(el_id)) will pass. We could do something like convert these to barns before checking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. I will consider this when I consolidate MacroXsCalculator where we probably use a common unit convention for all XsCalculator (next MR).

Comment on lines 99 to 103
// S-wave for neutron p < 14 MeV/c (kinetic energy < 0.1 MeV)
static CELER_CONSTEXPR_FUNCTION Momentum s_wave_limit()
{
return Momentum{14};
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how much it will matter, but it looks like the limit in Geant4 is only approximately 14 MeV/c (actually 13.568559 MeV/c, or log of momentum in GeV/c = -4.3).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. I changed it to 13.568559 even though it should be a rough value anyway.

@whokion
Copy link
Contributor Author

whokion commented Mar 7, 2024

@amandalund Thanks for additional review comments!

@sethrj sethrj enabled auto-merge (squash) March 7, 2024 21:28
@sethrj sethrj merged commit 1209964 into celeritas-project:develop Mar 7, 2024
20 of 21 checks passed
@whokion whokion deleted the neutron-elastic branch March 7, 2024 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request physics Particles, processes, and stepping algorithms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants