New curl2D implementation#373
Conversation
|
testCurl2D.m works now. |
| f = f + 1; | ||
| end | ||
| end | ||
| Dx = full(div(k, m, dx)); |
There was a problem hiding this comment.
full makes this untenable for large matrices.
There was a problem hiding this comment.
That is correct, we don't need to build the full matrices, we can just copy the divergence operator in the center of matrix with the two extra columns,
Dx = div(k, m, dx);
Dx = Dx(2:end-1,:);
| ks=[2,4,6,8]; | ||
| tolcomp12 = [5e-2,5e-4,5e-14,5e-14]; | ||
| tolcomp3 = [6e-2,3e-3,6e-14,6e-14]; | ||
| tolglob = [e-1,4e-3,8e-14,8e-14]; |
There was a problem hiding this comment.
This test generates an error "e not found"
There was a problem hiding this comment.
I had fixed this error in line 61, I re-commit the test.
There was a problem hiding this comment.
There was an indexing issue in the testCurl2D.m, it has been fixed and tests are now running fine.
There was a problem hiding this comment.
Pull request overview
This PR refactors the MATLAB/Octave curl2D operator to return a sparse matrix-based 2D curl operator (instead of computing curl from function handles), aligning with the “matrix operator” approach used elsewhere and supporting parity work for the C++ implementation (Issue #343).
Changes:
- Replaced
curl2D.mimplementation with a sparse matrix assembly using 1D divergence operators and Kronecker products. - Added a MATLAB unit test (
testCurl2D.m) intended to validate all three curl components against analytic derivatives.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/matlab_octave/curl2D.m |
Reimplements curl2D as a sparse matrix operator assembled from div-based stencils (via kron). |
tests/matlab_octave/testCurl2D.m |
Adds a unit test to compare the operator output against analytic curl components on a staggered grid. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ks=[2,4,6,8]; | ||
| tolcomp12 = [5e-2,5e-4,5e-14,5e-14]; | ||
| tolcomp3 = [6e-2,3e-3,6e-14,6e-14]; | ||
| tolglob = [e-1,4e-3,8e-14,8e-14]; |
| for k = ks | ||
| % tolerances | ||
| tolc12 = tolcomp12(k); | ||
| tolc3 = tolcomp3(k); | ||
| tolg = tolglob(k); |
| tolg = tolglob(k); | ||
|
|
||
| % 2D curl operator | ||
| C = curl2D(k, m, dx, n, dy); % need to modify the last component of 2D curl |
| @@ -1,62 +1,43 @@ | |||
| function C = curl2D(k, m, dx, n, dy, west, east, south, north, U, V) | |||
| % PURPOSE | |||
| % SPDX-License-Identifier: GPL-3.0-only | |||
There was a problem hiding this comment.
The header is different:
% ----------------------------------------------------------------------------
% SPDX-License-Identifier: GPL-3.0-or-later
% © 2008-2024 San Diego State University Research Foundation (SDSURF).
% See LICENSE file or https://www.gnu.org/licenses/gpl-3.0.html for details.
% ----------------------------------------------------------------------------| Dx = full(div(k, m, dx)); | ||
| Dx = sparse(Dx(2:end-1,:)); | ||
| Dy = full(div(k, n, dy)); | ||
| Dy = sparse(Dy(2:end-1,:)); |
A special character introduced strange behavior in the Markup code. There were some periods missing. Including links to textbooks.
jbrzensk
left a comment
jbrzensk
left a comment
There was a problem hiding this comment.
The code seems fine, the header though, is different from all of the matlab files.
| @@ -1,62 +1,43 @@ | |||
| function C = curl2D(k, m, dx, n, dy, west, east, south, north, U, V) | |||
| % PURPOSE | |||
| % SPDX-License-Identifier: GPL-3.0-only | |||
There was a problem hiding this comment.
The header is different:
% ----------------------------------------------------------------------------
% SPDX-License-Identifier: GPL-3.0-or-later
% © 2008-2024 San Diego State University Research Foundation (SDSURF).
% See LICENSE file or https://www.gnu.org/licenses/gpl-3.0.html for details.
% ----------------------------------------------------------------------------
|
I noticed this discrepancy too, but rather than manually editing the file, we have now an automatic mechanism in place which should have placed the standard license and texts. If you look at PR #342, the standardization for the license should have been automatically taken care of. The new labels are part of @deepanshu-zade's work that extends the work that @pritkc did. Now that Deepanshu is out for the Summer, @pritkc is helping us review this work. The goal is to automate the license standardization and propagation (for now to all MATLAB/Octave scripts and C++ Source files). In the future, we will have to move that work to other MOLE language modules. @pritkc is reviewing this work and will update the contributing guidelines. |
To comply with new contribution guidelines, the curl2D.m file has a new header comment which uses the documentation labels (PURPOSE, DESCRIPTION, SYNTAX, etc). The License is automatically included in the file.
|
@jbrzensk, I added the new documentation labels to the curl2D.m script file. The license is already automatically included. Let me know if you see it differently. Thanks! |
What type of PR is this? (check all applicable)
Description
Implements the 2D curl operator as a matrix instead of functions
Related Issues & Documents
QA Instructions, Screenshots, Recordings
tests/matlab_octave/testCurl2D.m
Added/updated tests?
_We encourage you to test all code included with MOLE, including examples.
Read Contributing Guide and Code of Conduct
[optional] Are there any post deployment tasks we need to perform?
[optional] What gif best describes this PR or how it makes you feel?