Skip to content

Spray data issue on hip #650

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.

Sign up for GitHub

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

Open
SreejithNREL wants to merge 11 commits into
base: development
Choose a base branch
from
Open

Spray data issue on hip #650

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
90ddd68
trying it on hip. compiles on mac
SreejithNREL Jan 24, 2026
6e01469
corrected the arena call
SreejithNREL Jan 24, 2026
e6c3df9
corrected thearena free call
SreejithNREL Jan 24, 2026
a567cd3
clang tidying
SreejithNREL Jan 28, 2026
4999515
removing the device_arena->free call
SreejithNREL Jan 29, 2026
4071c31
clangtidying
SreejithNREL Jan 29, 2026
7ffb7a1
Change downstream PeleLMeX CI executable name.
jrood-nrel Jan 30, 2026
24fe642
Merge branch 'development' into SprayDataIssueOnHip
baperry2 Feb 2, 2026
8e49e40
used nullptr
SreejithNREL Feb 2, 2026
fa19b40
clang tidy
SreejithNREL Feb 2, 2026
a746705
removed unused spray warning
SreejithNREL Feb 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/downstream.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,4 +312,4 @@ jobs:
echo "::add-matcher::${{ github.workspace }}/PeleC/.github/problem-matchers/gcc.json"
cat clang-tidy-warnings.txt
export return=$(tail -n 1 clang-tidy-warnings.txt | awk '{print $2}')
exit ${return}
exit ${return}
10 changes: 7 additions & 3 deletions Source/Spray/SprayInterpolation.H
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,8 @@ InterpolateGasPhase(
const amrex::IntVect* indx_array,
const amrex::Real* weights,
const pele::physics::eos::EosParm<pele::physics::PhysicsType::eos_type>*
eosparm = nullptr)
eosparm = nullptr,
const SprayData* spray = nullptr)
{

Copilot AI Feb 18, 2026

Copy link

Choose a reason for hiding this comment

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

When USE_MANIFOLD_EOS is enabled, this function unconditionally dereferences spray (e.g., spray->indx[n]), but the parameter default is nullptr. This makes it easy for any call site that doesn't pass spray to compile and then crash in manifold builds. Consider removing the default = nullptr for spray (forcing callers to provide it), or adding a #ifdef USE_MANIFOLD_EOS guard that aborts/asserts if spray == nullptr before dereferencing.

Suggested change
{
{
#ifdef USE_MANIFOLD_EOS
if (spray == nullptr) {
amrex::Abort(
"InterpolateGasPhase: null spray pointer with USE_MANIFOLD_EOS enabled");
}
#endif

Copilot uses AI. Check for mistakes.
auto eos = pele::physics::PhysicsType::eos(eosparm);
#ifdef PELELM_USE_SPRAY
Expand Down Expand Up @@ -164,11 +165,14 @@ InterpolateGasPhase(
// from the manifold table. We already have the interpolated value of manifold
// variables at the spray particle location stored in 'mass_frac'. I just need
// to call GenericReadManifoldData
#ifndef USE_MANIFOLD_EOS
amrex::ignore_unused(spray);
#endif

#ifdef USE_MANIFOLD_EOS
for (int n = 0; n < SPRAY_FUEL_NUM; n++) {
amrex::Real tempval;
eos.Y2ChemSpecies(
mass_frac.data(), SprayParticleContainer::d_sprayData->indx[n], tempval);
eos.Y2ChemSpecies(mass_frac.data(), spray->indx[n], tempval);
gpv.Yspec_fluid[n] = tempval;
}
#endif
Expand Down
6 changes: 4 additions & 2 deletions Source/Spray/SprayParticles.H
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,10 @@ public:

static void SprayCleanUp()
{
delete m_sprayData;
amrex::The_Arena()->free(d_sprayData);
// delete m_sprayData;
// amrex::The_Arena()->free(d_sprayData);
Comment on lines +48 to +49

Copilot AI Feb 18, 2026

Copy link

Choose a reason for hiding this comment

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

This cleanup now leaks both allocations: m_sprayData is never deleted and d_sprayData is never freed. Setting pointers to nullptr does not release memory. Since d_sprayData is now allocated with amrex::The_Device_Arena() in SpraySetup.cpp, it should be freed with amrex::The_Device_Arena()->free(d_sprayData) (not The_Arena()), and m_sprayData should still be deleted (or otherwise freed via the correct allocator if it was changed elsewhere).

Suggested change
// delete m_sprayData;
// amrex::The_Arena()->free(d_sprayData);
delete m_sprayData;
amrex::The_Device_Arena()->free(d_sprayData);

Copilot uses AI. Check for mistakes.
d_sprayData = nullptr;
m_sprayData = nullptr;
}

/// \brief Generalized injection routine for a single SprayJet
Expand Down
2 changes: 1 addition & 1 deletion Source/Spray/SprayParticles.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,7 @@ SprayParticleContainer::updateParticles(
gpv.reset();
InterpolateGasPhase(
gpv, state_box, rhoarr, rhoYarr, Tarr, momarr, engarr,
indx_array.data(), weights.data(), fdat->eosparm);
indx_array.data(), weights.data(), fdat->eosparm, fdat);
// Solve for avg mw and pressure at droplet location
fdat->calcBoilT(gpv, cBoilT.data());
if (is_film) {
Expand Down
7 changes: 5 additions & 2 deletions Source/Spray/SpraySetup.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,11 @@ SprayParticleContainer::readSprayParams(int& particle_verbose)
<< std::endl;
#endif
m_sprayData = new SprayData{};
d_sprayData =
static_cast<SprayData*>(amrex::The_Arena()->alloc(sizeof(SprayData)));
// d_sprayData =
// static_cast<SprayData*>(amrex::The_Arena()->alloc(sizeof(SprayData)));
Comment on lines +35 to +36

Copilot AI Feb 18, 2026

Copy link

Choose a reason for hiding this comment

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

The commented-out allocation block adds noise and can drift from reality over time (especially now that allocation/free must match arenas). Prefer deleting the commented code and leaving a short explanatory comment about why The_Device_Arena() is required for HIP/device access, or use a compile-time switch if different allocators are truly needed in different builds.

Suggested change
// d_sprayData =
// static_cast<SprayData*>(amrex::The_Arena()->alloc(sizeof(SprayData)));
// d_sprayData must be in device memory for HIP/GPU access, so allocate from
// The_Device_Arena and ensure deallocation uses the matching arena.

Copilot uses AI. Check for mistakes.
d_sprayData = static_cast<SprayData*>(
amrex::The_Device_Arena()->alloc(sizeof(SprayData)));

ParmParse pp("particles");
// Control the verbosity of the Particle class
pp.query("v", particle_verbose);
Expand Down
Loading