adaptive sstp: fix for the case when first step wwas not done in …#486
adaptive sstp: fix for the case when first step wwas not done in …#486pdziekan wants to merge 2 commits intoigfuw:masterfrom
Conversation
…pt and we only make one step
There was a problem hiding this comment.
Pull request overview
This PR adjusts the adaptive SSTP (substepping) condensation logic to correctly roll back the non-condensation state when the adaptation phase does not complete an initial condensation step and only a single trial step was applied.
Changes:
- Update the rollback multiplier used after the adaptation loop to handle the
sstp_cond_max == 1case. - Extend the inline comment describing the rollback behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // override number of substeps for SDs that de/activate in this timestep; | ||
| if(sstp_cond_act > 1) | ||
| { | ||
| const real_t rc2 = thrust::get<2>(thrust::get<2>(tpl)); | ||
|
|
||
| if ( ( rw2 < rc2 && (rw2 + sstp_cond * drw2) > rc2 ) || | ||
| ( rw2 > rc2 && (rw2 + sstp_cond * drw2) < rc2 ) ) | ||
| { | ||
| sstp_cond = sstp_cond_act; | ||
| first_cond_step_done_in_adaptation = false; | ||
| } | ||
| } | ||
| if(!first_cond_step_done_in_adaptation) | ||
| { | ||
| _apply_noncond_perparticle_sstp_delta(delta_fraction_applied); // revert to state before adaptation loop (beacause sstp_cond == sstp_cond_max and sstp_cond_max may not be a power of 2) | ||
| _apply_noncond_perparticle_sstp_delta(sstp_cond_max == 1 ? -delta_fraction_applied : delta_fraction_applied); // revert to state before adaptation loop (beacause sstp_cond == sstp_cond_max and sstp_cond_max may not be a power of 2); If only one step was tried, whole change was applied; If more steps were tried, we are moving back from the entire step |
There was a problem hiding this comment.
The revert logic here can leave sstp_tmp_* in a partially-advanced state when first_cond_step_done_in_adaptation was set to true in the convergence loop and then later flipped back to false by the activation override (sstp_cond_act block). In that path the code exits the loop with non-cond deltas net-applied for one substep (e.g., +1/sstp_cond), but delta_fraction_applied still reflects the last trial (e.g., -1/sstp_cond_try), so applying it once does not restore the pre-adaptation state and will bias subsequent substepping. Consider tracking the net fraction currently applied to sstp_tmp_* (or saving the pre-loop sstp_tmp_* values and restoring them) and reverting based on that net fraction instead of delta_fraction_applied/sstp_cond_max heuristics.
| if(!first_cond_step_done_in_adaptation) | ||
| { | ||
| _apply_noncond_perparticle_sstp_delta(delta_fraction_applied); // revert to state before adaptation loop (beacause sstp_cond == sstp_cond_max and sstp_cond_max may not be a power of 2) | ||
| _apply_noncond_perparticle_sstp_delta(sstp_cond_max == 1 ? -delta_fraction_applied : delta_fraction_applied); // revert to state before adaptation loop (beacause sstp_cond == sstp_cond_max and sstp_cond_max may not be a power of 2); If only one step was tried, whole change was applied; If more steps were tried, we are moving back from the entire step |
There was a problem hiding this comment.
Typo in the inline comment: "beacause" should be "because" (and the comment is quite long; consider splitting into two lines for readability).
| _apply_noncond_perparticle_sstp_delta(sstp_cond_max == 1 ? -delta_fraction_applied : delta_fraction_applied); // revert to state before adaptation loop (beacause sstp_cond == sstp_cond_max and sstp_cond_max may not be a power of 2); If only one step was tried, whole change was applied; If more steps were tried, we are moving back from the entire step | |
| _apply_noncond_perparticle_sstp_delta(sstp_cond_max == 1 ? -delta_fraction_applied : delta_fraction_applied); // revert to state before adaptation loop (because sstp_cond == sstp_cond_max | |
| // and sstp_cond_max may not be a power of 2); If only one step was tried, whole change was applied; If more steps were tried, we are moving back from the entire step |
…adapt and we only make one step