Conversation
Add BC-aware constructor overloads accepting Robin coefficient vectors dc (a0) and nc (b0). An axis is periodic when all dc/nc entries are zero, otherwise the existing non-periodic operator is used. Mixed periodicity supported in 2D/3D via per-axis checks. Adds two private static helpers: periodicGrad1D and isPeriodic. All original constructors unchanged.
Replace file(GLOB) with explicit source lists in src/cpp, tests/cpp, and examples/cpp CMakeLists.txt to avoid stale builds when files are added or renamed. Extend gradient.cpp with periodic boundary condition support: - Implement isPeriodic helper returning int instead of bool for efficiency - Remove Armadillo dependency in isPeriodic and zero-fill logic - Update dc and nc parameter types accordingly Update tests/cpp/test2.cpp to cover the periodic gradient case.
This reverts commit 5439812.
Add BC-aware constructor overloads accepting Robin coefficient vectors (dc/nc) for 1D, 2D, and 3D. Implement periodicGrad1D as a circulant sparse matrix via finite-difference stencil, with isPeriodic helper to detect periodic axes. Support mixed periodicity in 2D/3D through per-axis checking and Kronecker product assembly. Preserve backward compatibility by leaving original constructors untouched.
jbrzensk
left a comment
jbrzensk
left a comment
There was a problem hiding this comment.
Hi ya'll, thanks for this.
I tried it and it works appropriately. Most of the notes are on formatting. The clang-formatter flags almost every line, for spacing issues. It doesn't reject the PR but does generate a lot of errors that need to be addressed.
src/cpp/gradient.cpp
Outdated
| // first and last rows removed, leaving only the s interior rows. | ||
| static sp_mat trimmedIdentity(u32 s) { |
There was a problem hiding this comment.
Will this be used anywhere else? Maybe it should be in the utils.h?
There was a problem hiding this comment.
I agree, let me look into this. Thanks
There was a problem hiding this comment.
the trimmedIdentity function is currently used only for trimming in the gradient.cpp file, a similar operation is perform in the gradient.cpp but column-wise. The function was added to simply reduce code duplication.
A closer look, there are other sections of the gradient.cpp file that can also call this functions (with different arguments) and avoid duplications. For instance,
lines 242-245, and lines 268-273 of gradient.cpp.
@jbrzensk do you think that this would be useful to users as utility? or should it be kept internally?
There was a problem hiding this comment.
Its fine internal here as a static if nothing else could use it.
|
FYI, this work is being incrementally committed to allow for other team members to provide feedback as the work progresses. Thus, this Draft PR will have intermediate steps before becoming a PR. Other team members can incrementally review the C++ implementations of the three operators; gradient, divergence and laplacian in the feature/CPP_Parity_Ops branch. Here's a recap of the whole process:
|
|
@Jiya-Rathi @Tony-Drummond please add me as a reviewer on this PR. Thanks. |
Fixed formatting issues
|
The super-linter keeps flagging some errors at the end of some lines in both gradient.cpp and gradient.h, which are not visible when using at least 3 different editors (VS, emacs and vi). Most of all other formatting made by the linter suggestions have been adopted. Note that the real errors are flagged at the end of the linter run log summary. |
Tony-Drummond
force-pushed
the
feature/CPP_Parity_Ops
branch
3 times, most recently
from
54c22e2 to
3a8b53f
Compare
Jiya-Rathi
force-pushed
the
feature/CPP_Parity_Ops
branch
from
d0ff38b to
9651fb7
Compare
|
@jbrzensk @joehellmers @jcorbino the C++ gradient implementations are completed (most formatting issues corrected, and the new linter tests past). 🚀 test2.cpp has also been updated with new tests for periodic and non-periodic boundaries. If at least one of you approves, @Jiya-Rathi will merge the divergence implementations to this branch for incremental reviews. |
…ditions. It follows the MATLAB/Octave implementations. Added in gradient.cpp the function trimmedIdentity_rows to reuse code and avoid duplications. Updates to the gradient.cpp and grandient.h files. (1) [format] Removed trailing and extra spaces and some hidden characters (2) [format] added copyright symbol back (3) [delete] remove Jiya's #gradient.cpp# upstreamed by error (4) fixed copyright issues flagged by linters replacing the copyright symbol by (c) + Copyright + year. refactor(gradient): add build_gradient helper to eliminate clone Introduced build_gradient(G_m, I, k, dim, delta, periodic) to consolidate the repeated periodic/non-periodic axis setup blocks in the 2D and 3D BC-aware constructors. Declaration added to gradient.h. No behavioral change. Implement clang-tidy action. Update ci.yml Include arguments --format-style='llvm' --exclude-header-filter='third_party_install/.*' Update ci.yml
Tony-Drummond
force-pushed
the
feature/CPP_Parity_Ops
branch
from
cba0513 to
3830308
Compare
4 participants
What type of PR is this? (check all applicable)
Description
Currently the mimetic operators in MATLAB/Octave work for periodic and non-periodic boundary conditions. The C++ implementation needs to include functionality to support the periodic cases. The new MATLAB/Octave interface returns mimetic operators depending on whether or not the operator contain a periodic boundary condition type a0 U + b0 dU/dn = g. However, the current C++ gradient constructors do not check for these boundary conditions, nor has an implementation for periodic boundaries. We propose creating new C++ constructors that match the grad implementations in MATLAB/Octave.
Related Issues & Documents
QA Instructions, Screenshots, Recordings
Run test2.cpp to test this code.
Added/updated tests?
have not been included
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?