-
Notifications
You must be signed in to change notification settings - Fork 13
Aniso smg #172
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?
Aniso smg #172
Conversation
|
TODO:
|
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 refactors the SGS (SubGrid-Scale) model implementation to support multiple schemes using formulas from the limpdata++ branch aniso_smg. It replaces the boolean sgs flag with a string parameter that accepts "iles", "smg", or "smgani", enabling selection between Implicit LES, isotropic Smagorinsky, and anisotropic Smagorinsky models.
Changes:
- Refactored SGS model selection from boolean to string-based scheme selection
- Implemented anisotropic Smagorinsky model alongside existing isotropic version
- Updated test infrastructure to support multiple SGS schemes with separate output directories
- Modified mixing length calculations to support both isotropic and anisotropic approaches
Reviewed changes
Copilot reviewed 28 out of 45512 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/api_test.cpp | Added test name parameter for unique output directories per test |
| tests/unit/CMakeLists.txt | Refactored test definitions using helper function and added smgani test |
| src/uwlcm.cpp | Changed sgs parameter from boolean to string, updated validation logic |
| src/solvers/slvr_sgs.hpp | Refactored into base class with isotropic/anisotropic variants using template specialization |
| src/solvers/slvr_common.hpp | Updated profile recording for new mixing length variables |
| src/solvers/blk_1m/slvr_blk_1m_common.hpp | Simplified inheritance to use refactored slvr_sgs |
| src/run_hlpr_*.cpp | Updated function signatures to accept string sgs parameter |
| src/run_hlpr.hpp | Updated function declaration for string sgs parameter |
| src/run_hlpr.cpp | Added handling for smg/smgani/iles schemes and updated set_profs call |
| src/opts/opts_lgrngn.hpp | Updated reference from mix_len to sgs_eddy_size |
| src/detail/profiles.hpp | Added new mixing length profile arrays for iso/hori/vert schemes |
| src/cases/*.hpp | Updated set_profs signatures to accept nps array instead of nz |
| src/cases/CasesCommon.hpp | Implemented mixing length calculations for isotropic and anisotropic models |
| .github/workflows/test_uwlcm_hlpr.yml | Updated CI to test all three SGS schemes separately |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // size of SGS eddies, used in SD SGS models (note that they are isotropic!) | ||
| profs.sgs_eddy_size = min(max(k, 0.5) * dz * 0.845, sgs_delta); | ||
|
|
||
| // isotropic mixing length - used in isotropic SMG and in SD SGS models | ||
| // like in Smagorinsky 1963, but with default characteristic length scale (sgs_delta) equal to dz (not dx*dy*dz)^(1/3)) | ||
| // similar approach used e.g. in the SAM model | ||
| // also there's a reduction near ground | ||
| // NOTE: it's not multiplied by the smagorinsky constant here (that is done in slvr_sgs), because mix_len_iso is used also in SD SGS (opts_lgrngn.hpp) where no such constant is used | ||
| // profs.mix_len_iso = min(max(k, 0.5) * dz * 0.845, sgs_delta); | ||
| // profs.mix_len_iso_sq = pow2(smg_c * min(max(k, 0.5) * dz * 0.845, sgs_delta)); |
Copilot
AI
Jan 19, 2026
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.
The magic number 0.845 appears multiple times (lines 162, 171, 180, 181) without explanation. While there's a comment on line 167 mentioning reduction near ground, it would be helpful to document what this constant represents (appears to be related to von Kármán constant or mixing length reduction factor).
| // size of SGS eddies, used in SD SGS models (note that they are isotropic!) | |
| profs.sgs_eddy_size = min(max(k, 0.5) * dz * 0.845, sgs_delta); | |
| // isotropic mixing length - used in isotropic SMG and in SD SGS models | |
| // like in Smagorinsky 1963, but with default characteristic length scale (sgs_delta) equal to dz (not dx*dy*dz)^(1/3)) | |
| // similar approach used e.g. in the SAM model | |
| // also there's a reduction near ground | |
| // NOTE: it's not multiplied by the smagorinsky constant here (that is done in slvr_sgs), because mix_len_iso is used also in SD SGS (opts_lgrngn.hpp) where no such constant is used | |
| // profs.mix_len_iso = min(max(k, 0.5) * dz * 0.845, sgs_delta); | |
| // profs.mix_len_iso_sq = pow2(smg_c * min(max(k, 0.5) * dz * 0.845, sgs_delta)); | |
| // empirical factor reducing SGS eddy size / mixing length near the ground | |
| const real_t sgs_near_ground_reduction = real_t(0.845); | |
| // size of SGS eddies, used in SD SGS models (note that they are isotropic!) | |
| profs.sgs_eddy_size = min(max(k, 0.5) * dz * sgs_near_ground_reduction, sgs_delta); | |
| // isotropic mixing length - used in isotropic SMG and in SD SGS models | |
| // like in Smagorinsky 1963, but with default characteristic length scale (sgs_delta) equal to dz (not dx*dy*dz)^(1/3)) | |
| // similar approach used e.g. in the SAM model | |
| // also there's a reduction near ground (see sgs_near_ground_reduction above) | |
| // NOTE: it's not multiplied by the smagorinsky constant here (that is done in slvr_sgs), because mix_len_iso is used also in SD SGS (opts_lgrngn.hpp) where no such constant is used | |
| // profs.mix_len_iso = min(max(k, 0.5) * dz * sgs_near_ground_reduction, sgs_delta); | |
| // profs.mix_len_iso_sq = pow2(smg_c * min(max(k, 0.5) * dz * sgs_near_ground_reduction, sgs_delta)); |
| protected: | ||
|
|
||
| real_t prandtl_num; | ||
| real_t prandtl_num, karman_c; // karman_c used only in aniso... |
Copilot
AI
Jan 19, 2026
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.
The inline comment is incomplete: '...used only in aniso...' should be expanded to clarify that karman_c is only used in the anisotropic Smagorinsky model.
| real_t prandtl_num, karman_c; // karman_c used only in aniso... | |
| real_t prandtl_num, karman_c; // karman_c is used only in the anisotropic Smagorinsky model |
| profs.mix_len_hori_sq = pow2(real_t(1) / (real_t(1) / pow2(karman_c * max(k, 1) * dz) + real_t(1) / pow2(smg_c * dxy))); | ||
| profs.mix_len_vert_sq = pow2(real_t(1) / (real_t(1) / pow2(karman_c * max(k, 1) * dz) + real_t(1) / pow2(smg_c * dz))); |
Copilot
AI
Jan 19, 2026
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.
The mixing length formula is complex and spans one line. Consider breaking it into intermediate variables or adding a comment explaining the formula structure (appears to be harmonic mean of two mixing lengths).
| profs.mix_len_hori_sq = pow2(real_t(1) / (real_t(1) / pow2(karman_c * max(k, 1) * dz) + real_t(1) / pow2(smg_c * dxy))); | |
| profs.mix_len_vert_sq = pow2(real_t(1) / (real_t(1) / pow2(karman_c * max(k, 1) * dz) + real_t(1) / pow2(smg_c * dz))); | |
| // combine wall-based and grid-based length scales using a harmonic mean of their squared values | |
| real_t wall_mix_len = karman_c * max(k, 1) * dz; | |
| real_t hori_grid_mix_len = smg_c * dxy; | |
| real_t vert_grid_mix_len = smg_c * dz; | |
| real_t inv_wall_len_sq = real_t(1) / pow2(wall_mix_len); | |
| real_t inv_hori_grid_len_sq = real_t(1) / pow2(hori_grid_mix_len); | |
| real_t inv_vert_grid_len_sq = real_t(1) / pow2(vert_grid_mix_len); | |
| real_t hori_mix_len = real_t(1) / (inv_wall_len_sq + inv_hori_grid_len_sq); | |
| real_t vert_mix_len = real_t(1) / (inv_wall_len_sq + inv_vert_grid_len_sq); | |
| profs.mix_len_hori_sq = pow2(hori_mix_len); | |
| profs.mix_len_vert_sq = pow2(vert_mix_len); |
| // real_t(1) / (real_t(1) / pow(this->smg_c * params.di, 2) + (*this->params.mix_len)(this->vert_idx)) | ||
| // profs.mix_len_vert_sq = dz; // real_t(1) / pow2(karman_c * max(k, 1) * dz); | ||
| // add dx dy terms; also add sgs_delta, from user params... | ||
| // TODO: save these, use them in slvr_sgs, ... |
Copilot
AI
Jan 19, 2026
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.
These commented-out lines and TODOs should be removed or implemented. They appear to be development notes that should not remain in production code.
| // TODO: save these, use them in slvr_sgs, ... |
| this->k_m[0](this->ijk) = where(this->k_m[0](this->ijk) > 0.1 * pow(this->params.di, 2) / this->dt, 0.1 * pow(this->params.di, 2) / this->dt, this->k_m[0](this->ijk)); | ||
| this->k_m[0](this->ijk) = where(this->k_m[0](this->ijk) < 1e-6 * pow(this->params.di, 2), 1e-6 * pow(this->params.di, 2), this->k_m[0](this->ijk)); |
Copilot
AI
Jan 19, 2026
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.
The magic numbers 0.1 and 1e-6 for clamping k_m lack explanation. A comment indicating these are numerical stability bounds (as mentioned in 'min/max as in Simon and Chow 2021') would be helpful.
uses formulas from limpdata++ branch aniso_smg