Skip to content

revert relaxed root#2365

Merged
verheem merged 8 commits into
mainfrom
Revert-relaxed-root-implementation
Jun 12, 2025
Merged

revert relaxed root#2365
verheem merged 8 commits into
mainfrom
Revert-relaxed-root-implementation

Conversation

@verheem

@verheem verheem commented Jun 11, 2025

Copy link
Copy Markdown
Contributor

Fixes #2356 by reverting to a previous implementation first introduced at #1750

Achieved performance gain:

HollandseDelta_parameterized_2025_6_0
relaxed_root_old = Computation time: 1 minute, 17 seconds, 392 milliseconds
relaxed_root = Computation time: 3 minutes, 9 seconds, 247 milliseconds

@verheem
verheem requested a review from Copilot June 11, 2025 10:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the relaxed_root function’s threshold branch by replacing the original cubic smoothing polynomial and removing the intermediate x_scaled variable.

  • Replaced the previous x_scaled-based cubic expression with a new condensed formula.
  • Removed the x_scaled temporary variable.
Comments suppressed due to low confidence (2)

core/src/util.jl:713

  • There aren’t any tests covering the threshold branch of relaxed_root; please add unit tests around values near threshold to validate the smoothing behavior.
if abs(x) < threshold

core/src/util.jl:714

  • The new polynomial in the threshold branch no longer matches the original smoothing behavior and will produce incorrect values; please restore or correct the intended formula.
1 / 4 * (x / sqrt(threshold)) * (5 - (x / threshold)^2)

Comment thread core/src/util.jl
@verheem
verheem requested a review from SouthEndMusic June 12, 2025 10:45

@visr visr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good!

Copying what you mentioned in the chat here:

HollandseDelta_parameterized_2025_6_0
relaxed_root_old = Computation time: 1 minute, 17 seconds, 392 milliseconds
relaxed_root = Computation time: 3 minutes, 9 seconds, 247 milliseconds

Comment thread core/test/utils_test.jl Outdated
Co-authored-by: Martijn Visser <mgvisser@gmail.com>
@verheem
verheem merged commit f0cab91 into main Jun 12, 2025
8 of 10 checks passed
@verheem
verheem deleted the Revert-relaxed-root-implementation branch June 12, 2025 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ManningResistance at zero flow (equal Basins) seems slow

3 participants