Formatting files in /source/source_hamilt/module_xc#7399
Conversation
- Add tau_xc overload with hybrid_alpha parameter - Add ElecState::set_exx overload with cal_exx and hybrid_alpha parameters - Format code: one parameter per line, align code, initialize variables, one variable per line, add braces to single-line for/if
mohanchen
added
Refactor
- Format function parameters to one per line - Initialize all variables when declared - Separate variable declarations onto individual lines - Add braces for single-line if/for statements - Improve code alignment and readability
zzlinpku
left a comment
zzlinpku
left a comment
There was a problem hiding this comment.
Code Review Summary
This is a well-executed code style refactoring PR. The new file naming scheme (xc_*.cpp for native implementations, libxc_*.cpp for Libxc wrappers) is clear and consistent. A few items to address:
🔴 Must fix: Header guard in libxc.h still uses the old name.
🟡 Consider: Update comment references to old file names, and note the reference member risk in exx_info.h.
| #endif // USE_LIBXC | ||
|
|
||
| #endif // XC_FUNCTIONAL_LIBXC_H No newline at end of file | ||
| #ifndef XC_FUNCTIONAL_LIBXC_H |
There was a problem hiding this comment.
🔴 Issue: The header guard still uses the old filename XC_FUNCTIONAL_LIBXC_H. Since the file has been renamed from xc_functional_libxc.h to libxc.h, the guard should be updated accordingly.
| #ifndef XC_FUNCTIONAL_LIBXC_H | |
| #ifndef LIBXC_H | |
| #define LIBXC_H |
(The closing #endif on line 239 should also be updated to #endif // LIBXC_H)
| namespace XC_Functional_Libxc | ||
| { | ||
| //------------------- | ||
| // xc_functional_libxc.cpp |
There was a problem hiding this comment.
🟡 Nit: The section comment still references the old file name xc_functional_libxc.cpp. Consider updating to // libxc_setup.cpp to match the renamed file.
|
|
||
|
|
||
| //------------------- | ||
| // xc_functional_libxc_vxc.cpp |
There was a problem hiding this comment.
🟡 Nit: Same here — the old file name xc_functional_libxc_vxc.cpp should be updated to libxc_pot.cpp.
|
|
||
|
|
||
| //------------------- | ||
| // xc_functional_libxc_tools.cpp |
There was a problem hiding this comment.
🟡 Nit: Old file name xc_functional_libxc_tools.cpp → should be libxc_tools.cpp.
|
|
||
|
|
||
| //------------------- | ||
| // xc_functional_libxc_wrapper_xc.cpp |
There was a problem hiding this comment.
🟡 Nit: Old file name xc_functional_libxc_wrapper_xc.cpp → should be libxc_lda_wrap.cpp.
|
|
||
|
|
||
| //------------------- | ||
| // xc_functional_libxc_wrapper_tauxc.cpp |
There was a problem hiding this comment.
🟡 Nit: Old file name xc_functional_libxc_wrapper_tauxc.cpp → should be libxc_mgga_wrap.cpp.
| @@ -147,65 +155,91 @@ class XC_Functional | |||
| // LIBXC, and the reason for not having gcxc_libxc is explained | |||
| // in the NOTE in the comment for xc_functional_wrapper_wc.cpp part | |||
There was a problem hiding this comment.
🟡 Nit: The comment references xc_functional_wrapper_wc.cpp which appears to be a typo or old filename. Consider updating to xc_gga_wrap.cpp for clarity.
| double &v2c); | ||
|
|
||
| //------------------- | ||
| // xc_funct_hcth.cpp |
There was a problem hiding this comment.
🟡 Nit: Comment still references old filename xc_funct_hcth.cpp. Consider updating to xc_hcth.cpp.
| //XC_POLARIZED, XC_UNPOLARIZED: internal flags used in LIBXC, | ||
| //denote the polarized(nspin=1) or unpolarized(nspin=2) calculations, | ||
| //definition can be found in xc.h from LIBXC | ||
| void XC_Functional_Libxc::tau_xc( |
There was a problem hiding this comment.
✅ Nice design: The new 8-parameter tau_xc overload that accepts hybrid_alpha as a parameter is a clean improvement. The original 7-parameter version now delegates to it, which removes the direct dependency on GlobalC::exx_info from the core function. This makes the function more testable and reusable.
Consider applying the same pattern to tau_xc_spin if a hybrid mGGA functional needs it in the future.
| struct Exx_Info_RI | ||
| { | ||
| const std::map<Conv_Coulomb_Pot_K::Coulomb_Type, std::vector<std::map<std::string,std::string>>> &coulomb_param; | ||
| const std::map<Conv_Coulomb_Pot_K::Coulomb_Type, std::vector<std::map<std::string, std::string>>> &coulomb_param; |
There was a problem hiding this comment.
🟡 Note: Exx_Info_RI::coulomb_param is a const reference member bound to info_global.coulomb_param (line 82). This is a pre-existing design, but worth noting — if an Exx_Info_RI object outlives its source Exx_Info_Global, this becomes a dangling reference. Consider documenting this lifetime dependency or using a shared pointer if the ownership model changes in the future.
2 participants
Formatting files in /source/source_hamilt/module_xc