-
Notifications
You must be signed in to change notification settings - Fork 16
Dirichlet bcond #499
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: master
Are you sure you want to change the base?
Dirichlet bcond #499
Changes from 2 commits
521b164
7e7ffc7
09fea5e
dc7459d
666ebf6
c1690c6
bed3cdd
8ab7ce0
3bac12f
38ebf3c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -14,7 +14,7 @@ namespace libmpdataxx | |||||
| { | ||||||
| using namespace arakawa_c; | ||||||
|
|
||||||
| enum bcond_e { null, cyclic, polar, open, rigid, remote, gndsky, custom }; | ||||||
| enum bcond_e { null, cyclic, polar, open, rigid, remote, gndsky, fixed, custom }; | ||||||
| enum drctn_e { left, rght }; | ||||||
|
|
||||||
| template< | ||||||
|
|
@@ -81,6 +81,11 @@ namespace libmpdataxx | |||||
| assert(false && "bcond::save_edge_vel() called!"); | ||||||
| }; | ||||||
|
|
||||||
| virtual void save_edge_val(const arr_2d_t &, const arr_2d_t &, const rng_t &) | ||||||
pdziekan marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| { | ||||||
| assert(false && "bcond::save_edge_vel() called!"); | ||||||
|
||||||
| assert(false && "bcond::save_edge_vel() called!"); | |
| assert(false && "bcond::save_edge_val() called!"); |
Outdated
Copilot
AI
Dec 22, 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.
Missing parameter name in virtual function signature. The second parameter "const arr_3d_t &" lacks a descriptive name, making the function signature less readable. While this is valid C++, adding a parameter name like "val" would improve code documentation and consistency with the actual implementations in fixed_3d.hpp.
| virtual void save_edge_val(const arr_3d_t &, const rng_t &, const rng_t &) | |
| virtual void save_edge_val(const arr_3d_t &val, const rng_t &i, const rng_t &j) |
Outdated
Copilot
AI
Dec 22, 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.
Inconsistent error message in assertion. The assertion message says "bcond::save_edge_vel() called!" but this is actually the save_edge_val function. The error message should be "bcond::save_edge_val() called!" to accurately reflect which function was called.
| assert(false && "bcond::save_edge_vel() called!"); | |
| assert(false && "bcond::save_edge_val() called!"); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| // 2D fixed (Dirichlet) boundary conditions for libmpdata++ | ||
| // | ||
| // licensing: GPU GPL v3 | ||
pdziekan marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| // copyright: University of Warsaw | ||
|
|
||
| #pragma once | ||
|
|
||
| #include <libmpdata++/bcond/detail/bcond_common.hpp> | ||
|
|
||
| namespace libmpdataxx | ||
| { | ||
| namespace bcond | ||
| { | ||
| template <typename real_t, int halo, bcond_e knd, drctn_e dir, int n_dims, int d> | ||
| class bcond< real_t, halo, knd, dir, n_dims, d, | ||
| typename std::enable_if< | ||
| knd == fixed && | ||
| dir == left && | ||
| n_dims == 2 | ||
| >::type | ||
| > : public bcond<real_t, halo, open, dir, n_dims, d> // TODO: in some cases (e.g. cloud chamber) we might want to inherit from rigid? | ||
| { | ||
| using parent_t = bcond<real_t, halo, open, dir, n_dims, d>; | ||
| using arr_t = blitz::Array<real_t, 2>; | ||
| using parent_t::parent_t; // inheriting ctor | ||
|
|
||
| // holds fixed wall values (equal to initial edge values) | ||
| std::unordered_map<const arr_t*, arr_t> edge_values; | ||
|
|
||
| public: | ||
|
|
||
| void fill_halos_sclr(arr_t &a, const rng_t &j, const bool deriv = false) | ||
| { | ||
| #if !defined(NDEBUG) | ||
| assert(edge_values.find(&a) != edge_values.end() && "fixed bcond: edge values not saved before filling halos"); | ||
| #endif | ||
|
|
||
| using namespace idxperm; | ||
| for (int i = this->left_halo_sclr.first(); i <= this->left_halo_sclr.last(); ++i) | ||
| { | ||
| a(pi<d>(i, j)) = edge_values[&a](pi<d>(0, j)); | ||
| } | ||
| } | ||
|
|
||
| void save_edge_val(const arr_t &a, const arr_t &val, const rng_t &j) | ||
| { | ||
| using namespace idxperm; | ||
| // assert(a.shape() == val.shape()); | ||
|
||
| auto s = a.shape(); | ||
| s[d] = 1; | ||
| edge_values.emplace(&a, arr_t(s)); | ||
|
||
| if constexpr(d == 0) edge_values[&a].reindexSelf({0, a.lbound(1)}); | ||
| else if constexpr(d == 1) edge_values[&a].reindexSelf({a.lbound(0), 0}); | ||
| edge_values[&a](pi<d>(0, j)) = val(pi<d>(this->left_edge_sclr, j)); | ||
| } | ||
| }; | ||
|
|
||
| template <typename real_t, int halo, bcond_e knd, drctn_e dir, int n_dims, int d> | ||
| class bcond< real_t, halo, knd, dir, n_dims, d, | ||
| typename std::enable_if< | ||
| knd == fixed && | ||
| dir == rght && | ||
| n_dims == 2 | ||
| >::type | ||
| > : public bcond<real_t, halo, open, dir, n_dims, d> | ||
| { | ||
| using parent_t = bcond<real_t, halo, open, dir, n_dims, d>; | ||
| using arr_t = blitz::Array<real_t, 2>; | ||
| using parent_t::parent_t; // inheriting ctor | ||
|
|
||
| // holds fixed wall values (equal to initial edge values) | ||
| std::unordered_map<const arr_t*, arr_t> edge_values; | ||
|
|
||
| public: | ||
|
|
||
| void fill_halos_sclr(arr_t &a, const rng_t &j, const bool deriv = false) | ||
| { | ||
| using namespace idxperm; | ||
| for (int i = this->rght_halo_sclr.first(); i <= this->rght_halo_sclr.last(); ++i) | ||
| { | ||
| a(pi<d>(i, j)) = edge_values[&a](pi<d>(0, j)); | ||
| } | ||
|
||
| } | ||
|
|
||
| void save_edge_val(const arr_t &a, const arr_t &val, const rng_t &j) | ||
| { | ||
| using namespace idxperm; | ||
| // assert(a.shape() == val.shape()); | ||
|
||
| auto s = a.shape(); | ||
| s[d] = 1; | ||
| edge_values.emplace(&a, arr_t(s)); | ||
|
||
| if constexpr (d == 0) edge_values[&a].reindexSelf({0, a.lbound(1)}); | ||
| else if constexpr(d == 1) edge_values[&a].reindexSelf({a.lbound(0), 0}); | ||
|
||
| edge_values[&a](pi<d>(0, j)) = val(pi<d>(this->rght_edge_sclr, j)); | ||
| } | ||
| }; | ||
| } // namespace bcond | ||
| } // namespace libmpdataxx | ||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,105 @@ | ||||||||||
| // 3D fixed (Dirichlet) boundary conditions for libmpdata++ | ||||||||||
| // | ||||||||||
| // licensing: GPU GPL v3 | ||||||||||
pdziekan marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||
| // copyright: University of Warsaw | ||||||||||
|
|
||||||||||
| #pragma once | ||||||||||
|
|
||||||||||
| #include <libmpdata++/bcond/detail/bcond_common.hpp> | ||||||||||
|
|
||||||||||
| namespace libmpdataxx | ||||||||||
| { | ||||||||||
| namespace bcond | ||||||||||
| { | ||||||||||
| template <typename real_t, int halo, bcond_e knd, drctn_e dir, int n_dims, int d> | ||||||||||
| class bcond< real_t, halo, knd, dir, n_dims, d, | ||||||||||
| typename std::enable_if< | ||||||||||
| knd == fixed && | ||||||||||
| dir == left && | ||||||||||
| n_dims == 3 | ||||||||||
| >::type | ||||||||||
| > : public bcond<real_t, halo, open, dir, n_dims, d> // TODO: in some cases (e.g. cloud chamber) we might want to inherit from rigid? | ||||||||||
| { | ||||||||||
| using parent_t = bcond<real_t, halo, open, dir, n_dims, d>; | ||||||||||
| using arr_t = blitz::Array<real_t, 3>; | ||||||||||
| using parent_t::parent_t; // inheriting ctor | ||||||||||
|
|
||||||||||
| // holds fixed wall values (equal to initial edge values) | ||||||||||
| std::unordered_map<const arr_t*, arr_t> edge_values; | ||||||||||
|
|
||||||||||
| public: | ||||||||||
|
|
||||||||||
| void fill_halos_sclr(arr_t &a, const rng_t &j, const rng_t &k, const bool deriv = false) | ||||||||||
| { | ||||||||||
| #if !defined(NDEBUG) | ||||||||||
| assert(edge_values.find(&a) != edge_values.end() && "fixed bcond: edge values not saved before filling halos"); | ||||||||||
| #endif | ||||||||||
|
|
||||||||||
| using namespace idxperm; | ||||||||||
| for (int i = this->left_halo_sclr.first(); i <= this->left_halo_sclr.last(); ++i) | ||||||||||
| { | ||||||||||
| a(pi<d>(i, j, k)) = edge_values[&a](pi<d>(0, j, k)); | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| void save_edge_val(const arr_t &a, const arr_t &val, const rng_t &j, const rng_t &k) | ||||||||||
| { | ||||||||||
| using namespace idxperm; | ||||||||||
| // assert(a.shape() == val.shape()); | ||||||||||
|
||||||||||
| // assert(a.shape() == val.shape()); | |
| #if !defined(NDEBUG) | |
| assert(a.shape() == val.shape()); | |
| #endif |
Outdated
Copilot
AI
Dec 22, 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.
Potential memory leak or redundant allocation with emplace. The code uses emplace(&a, arr_t(s)) which always tries to insert a new element even if the key already exists. If save_edge_val is called multiple times with the same array pointer, emplace will fail to insert but the temporary arr_t(s) is still constructed. Consider using try_emplace or checking if the key exists before creating the array, or using insert_or_assign if re-initialization is intended.
| edge_values.emplace(&a, arr_t(s)); | |
| if (edge_values.find(&a) == edge_values.end()) | |
| edge_values.emplace(&a, arr_t(s)); |
Copilot
AI
Dec 22, 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.
Commented-out assertion should be either enabled or removed. Line 94 contains a commented assertion to verify that array shapes match. If this check is needed, it should be enabled within an #if !defined(NDEBUG) block like the assertion in the left direction implementation. If the check is not needed, the commented code should be removed to reduce clutter and improve maintainability.
| // assert(a.shape() == val.shape()); | |
| #if !defined(NDEBUG) | |
| assert(a.shape() == val.shape()); | |
| #endif |
Outdated
Copilot
AI
Dec 22, 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.
Potential memory leak or redundant allocation with emplace. The code uses emplace(&a, arr_t(s)) which always tries to insert a new element even if the key already exists. If save_edge_val is called multiple times with the same array pointer, emplace will fail to insert but the temporary arr_t(s) is still constructed. Consider using try_emplace or checking if the key exists before creating the array, or using insert_or_assign if re-initialization is intended.
| edge_values.emplace(&a, arr_t(s)); | |
| edge_values.try_emplace(&a, s); |
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.
Commented-out code section should be removed. Lines 66-79 contain a commented-out C++14 compatibility check that is no longer needed since the codebase has been upgraded to C++17. Instead of commenting out this code, it should be completely removed to improve code maintainability and reduce clutter.