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

Uses spline order in the energy loss calculation #1496

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

lebuller
Copy link
Contributor

Adds an input option in the runner, spline_eloss_order, which is passed to PhysicsParams::Input as an option. This option is used to define the order of interpolation to be preformed in calc_mean_energy_loss in PhysicsStepUtils.hh. If this order is 1 the existing linear cross-section calculator is used and if it is greater than 1 a spline calculator of the given order is used.

Copy link

github-actions bot commented Nov 12, 2024

Test summary

 3 844 files   5 953 suites   4m 8s ⏱️
 1 599 tests  1 571 ✅ 28 💤 0 ❌
19 824 runs  19 749 ✅ 75 💤 0 ❌

Results for commit 9532d2f.

♻️ This comment has been updated with latest results.

@sethrj sethrj added enhancement New feature or request physics Particles, processes, and stepping algorithms labels Nov 12, 2024
lbu added 3 commits November 20, 2024 16:03
…pline template. Failing due to energy grid of size 2 in test
…s in MockProcess. Also alter scattering mock process to have 3 xs points instead of 2
@lebuller lebuller marked this pull request as ready for review November 21, 2024 21:24
@sethrj sethrj self-requested a review December 2, 2024 14:02
builders[ValueGridType::energy_loss]
= std::make_unique<ValueGridLogBuilder>(
applic.lower.value(),
applic.upper.value(),
VecDbl{eloss_rate.value(), eloss_rate.value()});
VecDbl{eloss_rate.value(), eloss_rate.value(), eloss_rate.value()});
Copy link
Member

Choose a reason for hiding this comment

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

At this point it might be clearer to use the size/fill constructor

Suggested change
VecDbl{eloss_rate.value(), eloss_rate.value(), eloss_rate.value()});
VecDbl(3, eloss_rate.value()));

@@ -88,6 +88,7 @@ void from_json(nlohmann::json const& j, RunnerInput& v)
LDIO_LOAD_OPTION(max_steps);
LDIO_LOAD_REQUIRED(initializer_capacity);
LDIO_LOAD_REQUIRED(secondary_stack_factor);
LDIO_LOAD_OPTION(spline_eloss_order);
Copy link
Member

Choose a reason for hiding this comment

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

If you scroll down in the file, you'll see there's also a "save" that you should add as well.

@@ -265,7 +268,8 @@ struct PhysicsParamsScalars
== static_cast<bool>(fixed_step_action))
&& lambda_limit > 0 && range_factor > 0 && range_factor < 1
&& safety_factor >= 0.1
&& step_limit_algorithm != MscStepLimitAlgorithm::size_;
&& step_limit_algorithm != MscStepLimitAlgorithm::size_
&& spline_eloss_order > 0;
Copy link
Member

Choose a reason for hiding this comment

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

Move this check to right before lambda_limit > 0 so that the ordering is the same as the variables.

* \c spline_eloss_order: the order of interpolation to be used for the
* spline interpolation. If it is 1, then the existing linear interpolation
* is used. If it is 2+, the spline interpolation is used for energy loss using
* the specified order. Default value is 1
Copy link
Member

Choose a reason for hiding this comment

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

Reindent please (guessing this is VSCode's fault...)

eloss = Energy{step * calc_eloss_rate(pre_step_energy)};

size_type order = physics.scalars().spline_eloss_order;
CELER_ASSERT(order > 0);
Copy link
Member

Choose a reason for hiding this comment

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

Since we already asserted by construction that the physics is "true", let's skip this check.

Comment on lines +208 to +209
else if (order > 1)
{
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be:

Suggested change
else if (order > 1)
{
else
{

unless you want to add an else { CELER_ASSERT_UNREACHABLE(); } after the else.

// Construct a grid calculator from a physics table
template<class T>
inline CELER_FUNCTION T make_calculator(ValueGridId,
const size_type order) const;
Copy link
Member

Choose a reason for hiding this comment

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

Generally we don't const local function arguments, particularly because this can affect Windows ABI

Suggested change
const size_type order) const;
size_type order) const;

*/
template<class T>
CELER_FUNCTION T PhysicsTrackView::make_calculator(ValueGridId id,
const size_type order) const
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
const size_type order) const
size_type order) const

@sethrj
Copy link
Member

sethrj commented Dec 2, 2024

Great work @lebuller ! There are just a few things to look at.

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.

2 participants