Update the output of OFDFT, particularly for the ML part#7383
Update the output of OFDFT, particularly for the ML part#7383mohanchen wants to merge 4 commits into
Conversation
Summary of changes: 1. Modified ML_Base::set_device() to accept std::ostream& ofs_running parameter instead of using std::cout directly 2. Updated KEDF_ML::set_para() to pass ofs_running through the call chain 3. Modified KEDF_ML::init_data() to accept ofs_running parameter 4. Updated NN_OFImpl constructor to accept ofs_running parameter for logging nnode/nlayer 5. Modified Cal_MLKEDF_Descriptors::set_para() to accept ofs_running parameter for logging nkernel 6. Updated ML_EXX class methods (set_para, init_data, localTest) to use ofs_running 7. Updated all call sites to pass GlobalV::ofs_running 8. Changed 'NN' to 'Neural Network' in device initialization messages 9. Fixed 'WARNING: ML >= TF' message in KEDF_Manager::get_energy() to use ofs_running 10. Reformatted KEDF_ML::set_para() and cal_tool->set_para() calls with one parameter per line All ML KEDF related output messages now write to the running log file instead of stdout.
mohanchen
added
Refactor
zzlinpku
left a comment
zzlinpku
left a comment
There was a problem hiding this comment.
Thanks for this PR! The direction of redirecting diagnostic output from std::cout to the log stream (GlobalV::ofs_running) is exactly right — computational libraries should not pollute stdout with debug/info messages. The const std::string& improvement in set_device and the "NN" → "Neural Network" rename are also nice touches.
I left a few inline comments below. The main items to address:
- Unused parameter warning in
pot_ml_exx_label.cpp—ofs_runningis added toinit_data()but not used in the function body. - Inconsistent leading space in two files where
" Default type: "has a leading space that was not in the original message. - Typo — "unaviable" → "unavailable".
Also noted (not in diff, worth checking):
kedf_ml.cpp:140(get_energy) still has a barestd::cout << "energy" << energy << std::endl;that was not converted. If it's in scope for this PR, consider passingofs_runningto that method as well.ml_base.cpp:206,214(dumpTensor,dumpMatrix) still usestd::cout— these may be out of scope, but flagging for awareness.
| const std::vector<int> &of_ml_tanhp_nl, | ||
| const std::vector<int> &of_ml_tanhq_nl | ||
| const std::vector<int> &of_ml_tanhq_nl, | ||
| std::ostream& ofs_running |
There was a problem hiding this comment.
The ofs_running parameter is added to init_data() but does not appear to be used in the function body — the diff shows no std::cout → ofs_running conversions in this file. This will trigger a -Wunused-parameter compiler warning.
Note that there are two commented-out std::cout lines in this function (lines 157-158 in the original) that could be un-commented and converted to use ofs_running. Or, if the parameter is intentionally unused here (added only for interface consistency), consider commenting out the parameter name to suppress the warning:
| std::ostream& ofs_running | |
| std::ostream& /* ofs_running */ |
| torch::set_default_dtype(caffe2::TypeMeta::fromScalarType(torch::kDouble)); | ||
| auto output = torch::get_default_dtype(); | ||
| std::cout << "Default type: " << output << std::endl; | ||
| ofs_running << " Default type: " << output << std::endl; |
There was a problem hiding this comment.
There's an inconsistent leading space before Default type: that was not present in the original std::cout version. Other messages in this same file (e.g., "ninput = ...", "load net done", "feg_net_F = ...") do not have this leading space.
| ofs_running << " Default type: " << output << std::endl; | |
| ofs_running << "Default type: " << output << std::endl; |
| torch::set_default_dtype(caffe2::TypeMeta::fromScalarType(torch::kDouble)); | ||
| auto output = torch::get_default_dtype(); | ||
| std::cout << "Default type: " << output << std::endl; | ||
| ofs_running << " Default type: " << output << std::endl; |
There was a problem hiding this comment.
Same inconsistent leading space as in pot_ml_exx.cpp. Other messages in this file do not have a leading space.
| ofs_running << " Default type: " << output << std::endl; | |
| ofs_running << "Default type: " << output << std::endl; |
| else | ||
| { | ||
| std::cout << "--------------- Warning: GPU is unaviable ---------------" << std::endl; | ||
| std::cout << "------------------- Running NN on CPU -------------------" << std::endl; |
There was a problem hiding this comment.
Typo: "unaviable" should be "unavailable". Since you're already touching this line, it would be a good opportunity to fix it.
| std::cout << "------------------- Running NN on CPU -------------------" << std::endl; | |
| ofs_running << "--------------- Warning: GPU is unavailable ---------------" << std::endl; |
2 participants
Update the output of OFDFT, particularly for the ML part