Refactor sweep functions and remove unused code #251
Conversation
Refactor sweep functions and consolidate logic for handling boundary conditions. Remove unused sweep4 function and update sweep3 to handle circular boundaries more effectively.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors sweep functions to simplify boundary condition handling by consolidating multiple sweep implementations into a single function. The key change is renaming sweep3 to sweep and removing two unused functions (sweep, sweep2, and sweep4) that handled boundary conditions with less flexible approaches.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Removed commented-out code for wind direction quadrants and solution matrix calculations.
* Avalanching numba patch Tested for 2D cases, seems to work fine now. Need to test for 1D cases. * updated avalanching.py to include non erodible layers. Not tested yet. * Remove theta_stat and update theta_dyn description * removed static angle of repose. Could be added back later if needed. * Update aeolis/avalanching.py copilot suggested changes on memory allocation Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update aeolis/avalanching.py added boundary definition. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update aeolis/avalanching.py copilot suggestion to filter for ne Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update to work with NE layer in avalanche module Removed requirement for ne_file when avalanching is enabled * Update aeolis/avalanching.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update aeolis/constants.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Tested for circular boundaries.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # # plot Ct | ||
| # import matplotlib.pyplot as plt | ||
| # plt.imshow(quad[:10,:10], origin='lower') | ||
| # # plt.colorbar() | ||
| # plt.title('Concentration after %d sweeps' % k) | ||
| # plt.show() | ||
| # plt.imshow(Ct[:50,:50], origin='lower') | ||
| # # plt.colorbar() | ||
| # plt.title('Concentration after %d sweeps' % k) | ||
| # plt.show() | ||
| # plt.plot(pickup[0,:,0]) | ||
| # plt.plot(pickup[-1,:,0]) | ||
| # plt.show() |
There was a problem hiding this comment.
Large block of commented-out debugging/visualization code should be removed to improve code cleanliness.
small typos and minor code changes proposed by copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ython into Sierd-patch-1
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
aeolis/avalanching.py:41
- Function name contains typo: 'angele_of_repose' should be 'angle_of_repose'. Note this is pre-existing but worth fixing if renaming is feasible.
def angele_of_repose(s,p):
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Ct[:,0,0],Ct[:,-1,0] = np.average(Ct[:,-2,0]),np.average(Ct[:,1,0]) | ||
| # print(Ct[:,0,0]) | ||
| # print(Ct[:,-1,0]) | ||
| Ct[:,0,0],Ct[:,-1,0] = np.mean(Ct[:,-2,0]), np.mean(Ct[:,1,0]) |
There was a problem hiding this comment.
Changed from np.average to np.mean. While both produce the same result when no weights are provided, ensure this is intentional. If the original use of np.average was deliberate (e.g., for future weighted averaging), document the reason for the change.
| ufs[:,-2,:] = ufs[:,-1,:] | ||
| ufs[1,:,:] = ufs[0,:,:] | ||
| ufs[-2,:,:] = ufs[-1,:,:] | ||
| # now make sure that there is no gradients at the boundaries |
There was a problem hiding this comment.
Grammar issue: 'there is no gradients' should be 'there are no gradients'.
| # now make sure that there is no gradients at the boundaries | |
| # now make sure that there are no gradients at the boundaries |
| flux_down[i, j, 0] = slope_diff[i, j] * grad_h_down[i, j, 0]# / grad_h[i, j] | ||
| flux_down[i, j, 1] = slope_diff[i, j] * grad_h_down[i, j, 1]# / grad_h[i, j] |
There was a problem hiding this comment.
Commented-out division by grad_h[i, j] lacks explanation. If this normalization was intentionally removed, document why in a proper comment. If it's temporary, add a TODO. This affects the flux computation and could impact results.
| flux_down[i, j, 0] = slope_diff[i, j] * grad_h_down[i, j, 0]# / grad_h[i, j] | |
| flux_down[i, j, 1] = slope_diff[i, j] * grad_h_down[i, j, 1]# / grad_h[i, j] | |
| # The division by grad_h[i, j] was removed from the flux computation below. | |
| # TODO: Investigate if normalization by grad_h[i, j] is necessary for correct flux calculation. | |
| flux_down[i, j, 0] = slope_diff[i, j] * grad_h_down[i, j, 0] # / grad_h[i, j] | |
| flux_down[i, j, 1] = slope_diff[i, j] * grad_h_down[i, j, 1] # / grad_h[i, j] |
|
@copilot the steadystate solver is not compatible with using multiple sediment fractions. Implement a warning that stops the program after reading the input if this is the case. |
…ractions (#258) * Initial plan * Add validation to prevent steadystate solver with multiple sediment fractions Co-authored-by: Sierd <14054272+Sierd@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Sierd <14054272+Sierd@users.noreply.github.com>
Refactor sweep functions and consolidate logic for handling boundary conditions. Remove unused sweep4 function and update sweep3 to handle circular boundaries more effectively.