minor fixes to issues detected on fugaku installation#484
minor fixes to issues detected on fugaku installation#484pdziekan wants to merge 1 commit intoigfuw:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR applies small portability fixes aimed at addressing issues encountered during Fugaku installation, touching GPU/host math usage and ice nucleation temperature calculation.
Changes:
- Adjusts
absusage in adaptive condensation convergence checks for non-NVCC builds. - Modifies the mineral INP freezing-temperature inverse CDF return expression.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/impl/condensation/perparticle/perparticle_nomixing_adaptive_sstp_cond.ipp |
Updates how abs is brought into scope and used in convergence logic for adaptive substepping. |
include/libcloudph++/common/ice_nucleation.hpp |
Changes the T_freeze_CDF_inv return expression for mineral INPs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| return (real_t(273.15) + (real_t(8.934) - log(- log(real_t(1.) - rand) / A) ) / real_t(0.517)) * si::kelvin; | ||
| return real_t(real_t(273.15) + (real_t(8.934) - log(- log(real_t(1.) - rand) / A) ) / real_t(0.517)) * si::kelvin; | ||
| } |
There was a problem hiding this comment.
This return statement appears to be missing a closing ) for the outer real_t( cast, which will fail to compile. Even with the parenthesis fixed, casting the unit-bearing expression to real_t would be incorrect here since the function returns quantity<si::temperature, real_t>; the previous form (scalar) * si::kelvin is the appropriate pattern.
| && cuda::std::abs(drw2 < sstp_cond_adapt_drw2_max * rw2)) // otherwise for small droplets (near activation?) drw2_new == 2*drw already for 2 substeps, but we ativate too many droplets | ||
| // if(cuda::std::abs(drw2_new * 2 - drw2) <= tol * drw2) // drw2 converged | ||
| if((abs(drw2_new * 2 - drw2) <= sstp_cond_adapt_drw2_eps * rw2) // drw2 relative to rw2 converged | ||
| && abs(drw2 < sstp_cond_adapt_drw2_max * rw2)) // otherwise for small droplets (near activation?) drw2_new == 2*drw already for 2 substeps, but we ativate too many droplets |
There was a problem hiding this comment.
The second convergence condition applies abs() to the result of a comparison (drw2 < sstp_cond_adapt_drw2_max * rw2), so it effectively becomes abs(bool) (0/1) rather than bounding the magnitude of drw2. This likely breaks the intended safeguard for small droplets; consider comparing abs(drw2) (or abs(drw2_new) as appropriate) against sstp_cond_adapt_drw2_max * rw2 instead.
| && abs(drw2 < sstp_cond_adapt_drw2_max * rw2)) // otherwise for small droplets (near activation?) drw2_new == 2*drw already for 2 substeps, but we ativate too many droplets | |
| && (abs(drw2_new) < sstp_cond_adapt_drw2_max * rw2)) // otherwise for small droplets (near activation?) drw2_new == 2*drw already for 2 substeps, but we ativate too many droplets |
No description provided.