Skip to content

Cusolver: Internal review version - #3

Open
haxushu wants to merge 597 commits into
developfrom
cusolver
Open

Cusolver: Internal review version#3
haxushu wants to merge 597 commits into
developfrom
cusolver

Conversation

@haxushu

@haxushu haxushu commented Mar 10, 2022

Copy link
Copy Markdown
Owner

Internal review version

@darelbeida

Copy link
Copy Markdown
Collaborator

comment one

@darelbeida darelbeida left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test

GlobalC::wf.npwx));
}
if(m == 1)
if(m != 1)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

可以单独提一个PR修这个错误

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread source/src_pw/energy.cpp
}
else if(GlobalV::KS_SOLVER=="cusolver")
{
label = "CU";

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

缩进

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

上面的缩进两种风格都有😂
image

Comment thread source/src_pdiag/pdiag_double.h Outdated
#endif



Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

空行数-2

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment thread source/src_pdiag/pdiag_double.cpp Outdated
int maxncol;

MPI_Allreduce(&ncol, &maxncol, 1, MPI_INT, MPI_MAX, comm_2D);
// MPI_Reduce(&ncol, &maxncol, 1, MPI_LONG, MPI_MAX, 0, comm_2D);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

注释清理一下

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment thread source/src_pdiag/pdiag_double.cpp Outdated
#include "../module_base/memory.h"
#include "../module_base/timer.h"

#include <chrono>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

关于chrono这部分应该都是调试的,可以在适当时候清理掉

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have been removed.

Comment thread source/input.cpp Outdated
{

#ifdef __MPI
GlobalV::ofs_warning << "Cusolver is now only available for LCAO." << std::endl;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

全局的cusolver可以统一一下大小写,感觉和后面的写法不太一致,找一个统一的写法

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我不是说和ks_solver里面的字符串内容,我是说后面又看到了cuSolver写法,与这里的Cusolver不一致

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

后面都是全小写的,这里因为是一个句子,首字母大写了一下。

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

除了这个我后面都check统一一下

int iic;

if(GlobalV::KS_SOLVER=="genelpa" || GlobalV::KS_SOLVER=="scalapack_gvx") // save the matrix as column major format
if(GlobalV::KS_SOLVER=="genelpa" || GlobalV::KS_SOLVER=="scalapack_gvx" || GlobalV::KS_SOLVER=="cusolver") // save the matrix as column major format

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个判断后面用了非常多次,能不能新构造一个GlobalV的变量记录一下这个布尔值?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

变量名可能和列优先相关

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

abacus对genelpa和scalapack_gvx都没有这样做,感觉判断的代价不大。

Comment thread source/module_cusolver/diag_cusolver.cu Outdated
int Diag_cuSolver_gvd::Dngvd_double(int N, int M, double *A, double *B, double *W, double *V){

copy_double(N, M, A, B);
buffer_double();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这步有办法只做第一次吗,后面只需要判断形状是否与之前一致,一致就不重复做,否则销毁d_work并重新做

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

补充一下,这个我评论错了,需要改在source/src_pdiag/diag_cusolver.cu里,而不是这个为了测试而存在的文件里,需要特别注意

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cusolverDnDsygvd_bufferSize不清楚内部机制,不能确定是否一致。

Comment thread source/module_cusolver/diag_cusolver.cu Outdated
}
int Diag_cuSolver_gvd::Dngvd_double(int N, int M, double *A, double *B, double *W, double *V){

copy_double(N, M, A, B);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我建议copyrecopy用上CHECK_CUDA宏之后,可以直接把代码写在这个函数里,加上一行注释即可,不需要单独的类方法。

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compute也是,核心只有一个cusolverDnDsygvd调用和一个sync,足够简短可以写到这个函数里面来

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment thread source/module_cusolver/diag_cusolver.cu Outdated
}


void Diag_cuSolver_gvd::finalize(){

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

和析构函数合并起来吧,看起来并没有除了析构以外的逻辑会调用它

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have accepted.

double eig[dim]; // reference

// construct the matrix A and B w.r.t the problem Ax = \lambda Bx
for (int i = 0; i < dim*dim; i++) a[i] = {1, 0};

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里能不能构造出一个带虚部的例子?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我再加一组

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have added to test complex_2.

Comment thread tests/module_cusolver/CMakeLists.txt Outdated

# compile the test executable
file(GLOB TEST_SRC
../../source/module_cusolver/test/cusolver_test.cpp

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我在测试的cpp文件内以及这里都没有看到对CUDA_LCAO或者MPI宏的判断,我觉得至少应该加在某处?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

考虑到单元测试的独立性,如果要分开测,可以在file(GLOB TEST_SRC单列cusolver_test.cpp或mpi.cpp

@haxushu
haxushu force-pushed the cusolver branch 2 times, most recently from d83df2b to a19a372 Compare March 25, 2022 12:04
caic99 and others added 5 commits April 4, 2022 15:35
fix : UT for diago_cg.cpp and diago_david.cpp should not depend on __CUDA and __ROCM
* fix : when input structure doesn't have most symmetry rotation matrix,
ABACUS will print which structure is better and then using the input structure for calculating.

* perf : add warning output for symmetry searching part; modified tests/integral/207_NO_DP_OD/result.ref to correct version.

* fix : UT for diago_cg.cpp and diago_david.cpp should not depend on __CUDA and __ROCM

* fix : support both element magnetization and atomic magnetization for initial

* add 304_NO_GO_AF_atommag case for testing initial atomic magnetization
dyzheng and others added 10 commits April 6, 2022 16:25
fix : support both element magnetization and atomic magnetization for initial setting in STRU file
…eepmodeling#120)

* fix : when input structure doesn't have most symmetry rotation matrix,
ABACUS will print which structure is better and then using the input structure for calculating.

* perf : add warning output for symmetry searching part; modified tests/integral/207_NO_DP_OD/result.ref to correct version.

* fix : UT for diago_cg.cpp and diago_david.cpp should not depend on __CUDA and __ROCM

* fix : support both element magnetization and atomic magnetization for initial

* add 304_NO_GO_AF_atommag case for testing initial atomic magnetization

Co-authored-by: Chun Cai <amoycaic@gmail.com>
synchronization between abacusmodeling and deepmodeling abacus-develop/develop branch
range: source/src_pw/test/diago_david_test.cpp
1. update Makefile.system and Makefile.vars
wenfei-li and others added 4 commits May 10, 2022 15:04
gint : removed obsolete code; implement new gamma force & stress
maki49 and others added 20 commits May 10, 2022 20:44
1. Fix the bug that multiple line heading of blocks <PP_R>, <PP_RAB>, <PP_DIJ>, and <PP_RHOATOM> in "PROJECT-Eu.UPF-1.txt" cannot be recognized by Pseudopot_upf::read_pseudo_upf201.
2. Release the restriction that off-diagonal elements of DIJ (Pseudopot_upf::dion) must be zero.
Fix: not using `DM[is]` when no atom found on grid
fix: Add the support to the multiple line heading in pseudopotential.
test(parallel): UT of methods in parallel_kpoints
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.