-
Notifications
You must be signed in to change notification settings - Fork 29
New Separation bubble #271
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
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 replaces the original polygon-based flow separation model with a new streamline-based separation bubble computation. The implementation uses Numba-accelerated cell-by-cell streamline computation with upwind curvature anticipation and geometry-based reattachment.
Key Changes:
- New streamline-based separation algorithm in
separation.pywith 1D/2D support - Refactored separation logic out of
shear.pyinto dedicated module - Updated parameter system with auto-tuning capability based on perturbation theory
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 16 comments.
| File | Description |
|---|---|
| aeolis/separation.py | New module implementing streamline-based separation bubble computation with Numba acceleration |
| aeolis/wind.py | Updated shear function calls to use new separation module; integrated 1D separation with new API |
| aeolis/shear.py | Removed old polygon-based separation method; integrated new separation module into FFT shear computation |
| aeolis/constants.py | Added new separation bubble parameters with auto-tuning support; removed legacy parameters |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| # ----- 1. UPWARD CURVATURE ANTICIPATION ----- | ||
|
|
||
| # Start looking forward for upward curvature anticipation |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: 'curviture' should be 'curvature'.
| # Required slope (s) to close height difference from (i) to (j) | ||
| s_req_height = dzj / dxj | ||
|
|
||
| # Required curviture (k) to close slope difference for height (k_req_height) and slope (k_req_slope) |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: 'Curviture' should be 'Curvature'.
| # Required curviture (k) to close slope difference for height (k_req_height) and slope (k_req_slope) | |
| # Required curvature (k) to close slope difference for height (k_req_height) and slope (k_req_slope) |
| s['zsep'] = zsep[np.newaxis, :] # shape back to (1, nx) | ||
| s['hsep'] = s['zsep'] - s['zb'] | ||
|
|
||
| # Compute influence of searation bubble on shear |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: 'searation' should be 'separation'.
| # Compute influence of searation bubble on shear | |
| # Compute influence of separation bubble on shear |
| look_n = int(look_dist / dx) | ||
| i_end = min(i + look_n, N - 1) | ||
|
|
||
| # Initialize maximum required curviture (k_req_max) and the resulting slope (v_z) |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: 'curviture' should be 'curvature'.
| 'sep_auto_tune' : True, # [-] Boolean for automatic tuning of separation bubble parameters based on characteristic length scales | ||
| 'sep_look_dist' : 50., # [m] Flow separation: Look-ahead distance for upward curvature anticipation | ||
| 'sep_k_press_up' : 0.05, # [-] Flow separation: Press-up curvature | ||
| 'sep_k_crit_down' : 0.18, # [1/m] Flow separation: Maximum downward curvature |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation: The unit notation [1/m] for sep_k_crit_down appears inconsistent with the unit-less [-] notation used for sep_k_press_up on line 251, even though both represent curvature parameters. Clarify whether these parameters have different units or if the notation should be consistent.
| 'sep_k_crit_down' : 0.18, # [1/m] Flow separation: Maximum downward curvature | |
| 'sep_k_crit_down' : 0.18, # [-] Flow separation: Maximum downward curvature |
| if udir < 0 and ny_ == 0: | ||
| z = z[::-1] |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Condition check uses ny_ == 0 but should check ny_ == 1 for 1D case. The shape of a 1D array would be (1, nx) with ny_ = 1, not ny_ = 0. Additionally, variable z is used but should be z_bed.
| k_req_height = (s_req_height - s_str) / dxj | ||
| k_req_slope = (sbj - s_str) / dxj | ||
|
|
||
| # Prevent that the streamline "overshoots" the bed level due a too steep curviture |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: 'curviture' should be 'curvature'.
| z_origin = gc['z'].copy() | ||
| if p['process_separation']: | ||
| z_origin = gc['z'][:].copy() | ||
| zsep = compute_separation(p, gc['z'], gc['dx']) |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API Design: The function compute_separation is called with positional arguments (p, gc['z'], gc['dx']) but in wind.py line 286, it's called with (p, z_bed, dx, udir). The parameter order differs (in wind.py, udir is the 4th parameter, while in shear.py only 3 parameters are passed). Verify that compute_separation has a default value for udir or that this inconsistency is intentional.
|
|
||
| # Compute shear velocity field (including separation) | ||
|
|
||
| # Quasi-2D shear model (DUNA (?) approach) |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unclear comment: The comment "Quasi-2D shear model (DUNA (?) approach)" contains a question mark suggesting uncertainty about the approach name. Either confirm the correct name or remove the uncertainty marker for clarity.
| # Quasi-2D shear model (DUNA (?) approach) | |
| # Quasi-2D shear model (DUNA approach) |
|
|
||
| # Flip the bed for negative wind directions (only in 1D) | ||
| if udir < 0 and ny_ == 0: | ||
| z = z[::-1] |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable z is not used.
| z = z[::-1] |
|
@bartvanwesten, I am testing this on my own cases. I want to be able to play with the dune growing in width and/or in height in combination with migration speed. This is a typical result where I vary wind direction and the old mu_b parameter. The site I am using has very limited interaction width the beach so the erosion and sedimentation are mostly governed by erosion at the dune face and deposition at the dune lee side. It is especially this lee side I am interested in. With the old code I cannot succeed making something like a plateau (dune growing wider but not higher...). What should i vary in the new code to manipulate the lee side?
|
|
@Sierd Unfortunately I cannot direclty reply to your message.. However, sep_s_leeside is the equivalent of the old mu_b, so you could try that. Another solution could be is to play around with values for L (in shear module). It strongly influences the "flatness" of landforms; typical range can go from 10-100 m. No idea of the new separation approach can solve this issue though. |
Fix ustarn calculation: initialization and FFT shear formula bugs (#265)

New Streamline-Based Separation Bubble
This PR introduces a complete replacement for the original polygon-based flow separation model in AeoLiS.
Key Improvements
Cell-by-cell streamline computation (Numba-accelerated).
Robust under complex terrain and multi-dune cases.
Upwind curvature anticipation.
Handles cliffs, sharp convexities, and multiple dunes.
Geometry-based reattachment.
Attachment length estimated from dune height + curvature requirement.
Curvature-driven detachment.
Enables separation on smaller features with modest slopes.
Refactored code
Removed separation functionality from shear.py and created well-documented code in separation.py.
ReadtheDocs
I included a more detailled description in this added markdown file. This can hopefully explain to you what's done and serve as a new section to the AeoLiS ReadtheDocs later on.
readthedocs_separation.md
Results
Validation
Tested on:
Current results are consistently smoother, and more robust.
Next Steps
@Sierd @ncohn Although testing is not entirely finished, I want to do this Pull Request now because: