Skip to content
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

Asadchev/refactor/math #230

Merged
merged 7 commits into from
Dec 14, 2020
Merged

Asadchev/refactor/math #230

merged 7 commits into from
Dec 14, 2020

Conversation

asadchev
Copy link
Contributor

@asadchev asadchev commented Dec 8, 2020

No description provided.

@asadchev asadchev requested a review from evaleev December 8, 2020 19:21
@asadchev asadchev changed the base branch from master to tot_devel December 8, 2020 19:22
@asadchev asadchev force-pushed the asadchev/refactor/math branch 12 times, most recently from 9cee529 to f544a8c Compare December 11, 2020 04:58
@@ -5,6 +5,8 @@
# Find dependencies related to ScaLAPACK
###################

add_compile_definitions( ${MPI_CXX_COMPILE_DEFINITIONS} )
Copy link
Member

Choose a reason for hiding this comment

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

how does this propagate to dependencies? should this be applied to a particular target?

Choose a reason for hiding this comment

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

Shouldn't we be using MPI::MPI_C (not MPI::MPI_CXX, that has some really dumb behaviour for OpenMPI) for this anyways?

@@ -5,6 +5,8 @@
# Find dependencies related to ScaLAPACK
###################

add_compile_definitions( ${MPI_CXX_COMPILE_DEFINITIONS} )

Choose a reason for hiding this comment

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

Shouldn't we be using MPI::MPI_C (not MPI::MPI_CXX, that has some really dumb behaviour for OpenMPI) for this anyways?

? madness::cblas::Trans
: madness::cblas::NoTrans);
const madness::cblas::CBLAS_TRANSPOSE right_op =
? blas::Transpose

Choose a reason for hiding this comment

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

We should probably be using blas::Op::Trans, blas::Op::NoTrans and blas:Op:::ConjTrans to make the transition to blaspp/lapackpp more transparent (e.g. typedef blas::Op Op;). I can make the necessary changes to blacspp and scalapackpp to be more consistent as well


enum TransposeFlag { NoTranspose, Transpose, ConjTranspose };

enum class SVDReturn {

Choose a reason for hiding this comment

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

I originally had these as strong types to allow for the possibility of typed template specialization for particular combinations (the performant alg choices for thin return / value only may be slightly different than full return, etc). I'm fine keeping the change, but we may find use cases for the strong types later on if we look at more advanced dispatch

if (world.rank() == 0) {
lapack::cholesky(A_eig);
linalg::rank_local::cholesky(A_eig);
}
world.gop.broadcast_serializable(A_eig, 0);

Choose a reason for hiding this comment

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

Cholesky can be safely replicated with identical inputs (not like XSYEV which has a phase problem), and since make_matrix already seems to be replicating the data, that would save you some communication / synchronization

World& world = A.world();
auto A_eig = detail::to_eigen(A);
auto A_eig = detail::make_matrix(A);

Choose a reason for hiding this comment

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

Since make_matrix replicates here, wouldn't it make more sense to add a "scatter/gather" type driver rather than "Alltoall` to save on some communication dependencies?

@asadchev asadchev force-pushed the asadchev/refactor/math branch 5 times, most recently from f25eafe to 9aa9ff3 Compare December 13, 2020 20:52
- Reorganize rank-local matrix, blas, and linalg
- Replace madness::cblas::Transpose by TA::blas::TransposeFlag
- Move non-distributed DistArray linalg to TiledArray/algebra/non-distributed
@asadchev asadchev force-pushed the asadchev/refactor/math branch 2 times, most recently from 191e631 to a70762d Compare December 14, 2020 17:11
@asadchev asadchev force-pushed the asadchev/refactor/math branch from a70762d to 11daf44 Compare December 14, 2020 19:21
@asadchev asadchev changed the base branch from tot_devel to master December 14, 2020 19:23
@asadchev asadchev merged commit fda0a2c into master Dec 14, 2020
@evaleev evaleev deleted the asadchev/refactor/math branch January 10, 2021 13:50
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.

3 participants