Add nuclear zone data #1269
Add nuclear zone data #1269
Conversation
whokion
added
enhancement
|
@sethrj Is the fail from windows real? |
| real_type integral, | ||
| real_type ws_shift) const | ||
| { | ||
| constexpr int max_trials = 1e+3; |
There was a problem hiding this comment.
@whokion This is the source of the windows error:
D:\a\celeritas\celeritas\src\celeritas\neutron\model\detail/NuclearZoneBuilder.hh(368): error C2220: the following warning is treated as an error
2024-06-12T13:55:56.9194698Z D:\a\celeritas\celeritas\src\celeritas\neutron\model\detail/NuclearZoneBuilder.hh(368): warning C4244: 'initializing': conversion from 'double' to 'const int', possible loss of data
Write it as 1000. (In theory you could specify a real number that's out of range of int and then loses accuracy.)
…lossing a nucleon
| // Average value of \f$ r^{2} V(r) dr \f$ at boundaries | ||
| real_type integral = half() * delta_r | ||
| * (rmin * (rmin + ws_shift) / (1 + std::exp(rmin)) | ||
| + rmax * (rmax + ws_shift) / (1 + std::exp(rmax))); |
There was a problem hiding this comment.
This (and the gaussian case below) are just initial guesses for the integral... this should be trivial to incorporate into the adaptive quadrature solver itself, right?
There was a problem hiding this comment.
You are absolutely right. This was a lazy man's implementation, but definitely can be integrated into PotentialIntegrator that you suggested early.
|
@sethrj Thanks a lot for your review comments and suggestions, which I hope that I addressed most of them except the request for PotentialIntegrator (which may be a separate MR). Is the CI failure something that I should worry about or a just glitch? |
|
@whokion Just a glitch I think: rerunning now. I'll see if I can make a small PR with a generic numerical integrator and test... |
There was a problem hiding this comment.
One or two small suggestions; I'll let @amandalund review and then I can follow on after the merge with the numerical integrator.
| size_type num_of_zones = components.size(); | ||
| std::vector<real_type> integral(num_of_zones); |
There was a problem hiding this comment.
Instead of doing the "number of zones" calculation in a separate function and expecting the size to match in the if statements below, I think it'd be better to resize the vector inside this function. Inside the if statements, resize the vector to 1, y.size(), alpha.size(), etc., and then loop over integral.size() rather than a separate num_of_zones values.
There was a problem hiding this comment.
Not sure about this. The number of zones is used for other properties (i.e., volumes, etc), so it may be still more readable keeping it as a common size across the whole code, which is called just once for a specific Isotope.
There was a problem hiding this comment.
Sorry, i didn't see this wasn't addressed. Are you expecting NuclearZoneBuilder::num_nuclear_zones to be called outside of that helper class? The number of zones is implicitly available through the components size, so the separate assumptions between num_nuclear_zones and calc_zone_component is a dangerous redundancy.
| * Device data for nuclear zone properties | ||
| */ | ||
| template<Ownership W, MemSpace M> | ||
| struct NuclearZoneData |
There was a problem hiding this comment.
I think the data could be simplified slightly by removing this struct and instead storing the components and zones directly in NeutronInelasticData.
There was a problem hiding this comment.
Actually, I originally did what you are suggesting, but I may want to reorganize data structure by grouping data separately needed for the process (i.e., macro_xs) and for the interactor (nucleon_xs, zone_data and other data which will be added later). Although NuclearZoneData is too specific now and can be renamed by the more generic name as more data are added, I would keep the structure as it is now and reorganize them later as more codes and data are added.
Extend neutron inelastic data for the Bertini cascade nuclei model
This MR adds data for nuclear zone properties to NeutronInelasticData and associated updates: