HSPotential Helper replace ifdefs with const expr templates #4623

walshmm · 2023-06-02T16:43:51Z

Please review the developer documentation
on the wiki of this project that contains help and requirements.

Proposed changes

This is a refactor of HSPotential_Helpers.cpp and HSPotential_Helpers.h in accordance with this gist https://gist.github.com/williamfgc/28ffbd23f6888ed89530e3f68c992c93#refactoring-strategy
It removes the QMC_COMPLEX ifdefs and replaces them with templates and const expr as to generate complex/real versions in one compile.

Describe what this PR changes and why. If it closes an issue, link to it here
with a supported keyword.

What type(s) of changes does this code introduce?

Delete the items that do not apply

Refactoring (no functional changes, no api changes)

Does this introduce a breaking change?

No

What systems has this change been tested on?

Checklist

Update the following with a yes where the items apply. If you're unsure about any of them, don't hesitate to ask. This is
simply a reminder of what we are going to look for before merging your code.

Yes. This PR is up to date with current the current state of 'develop'
Yes/No. Code added or changed in the PR has been clang-formatted
NA. This PR adds tests to cover any new code, or to catch a bug that is being fixed
NA. Documentation has been added (if appropriate)

prckent

LGTM

Will need a non-ORNL merge.

We should talk if there is anything tricky under AFQMC given our focus on the real space code.

prckent · 2023-06-02T18:08:45Z

Note: we are at the mercy of the AFMC test coverage here. Recommend you have
QMC_DATA/NiO/NiO_afm_fcidump.h5
QMC_DATA/NiO/NiO_afm_wfn.dat
QMC_DATA/NiO/NiO_nm_choldump.h5
which are only used by AFQMC tests when developing.

quantumsteve · 2023-06-02T18:50:03Z

src/AFQMC/config.0.h

+template <class T>
+struct is_complex : std::false_type {};
+template <>
+struct is_complex<ComplexType> : std::true_type {};
+template <>
+struct is_complex<SPComplexType> : std::true_type {};
+
+{};


Suggested change

template <class T>

struct is_complex : std::false_type {};

template <>

struct is_complex<ComplexType> : std::true_type {};

template <>

struct is_complex<SPComplexType> : std::true_type {};

{};

template <class T>

struct is_complex : std::false_type {};

template <class T>

struct is_complex<std::complex<T>> : std::true_type {};

template< class T>

inline constexpr bool is_complex_v = is_complex<T>::value;

quantumsteve · 2023-06-02T18:55:25Z

src/AFQMC/Hamiltonians/HSPotential_Helpers.cpp

-#if defined(QMC_COMPLEX)
-        count[2 * (*cik) + 1] += size_t(2); // Lik - 0
-#endif
+        if constexpr (is_complex<T>::value)


Suggested change

if constexpr (is_complex<T>::value)

if constexpr (is_complex_v<T>)

Requires the helper function I suggested adding in config.0.h

quantumsteve · 2023-06-02T19:30:08Z

src/AFQMC/Hamiltonians/HSPotential_Helpers.cpp

@@ -35,7 +35,7 @@ namespace
 {
 //inline double const& ma::conj(double const& d){return d;}
 //inline float const& ma::conj(float const& f){return f;}
-
+template<class T>


Since these functions are in the implementation file we need to explicitly instantiate them.

William's branch has a couple macros to make this easier.

I feel the template parameter should be bool instead of T. Then the explicit instantiation only needs true and false two times.

If we go that route, make it an enum or enum class. Names would be clearer than true and false and if in the future we want to add more template options (mixed precision?) there is less to change.

Good point @quantumsteve . We will want to add mixed precision support in future to do away with QMC_MIXED_PRECISION. If we can pick a pattern that make both r/c and sp/dp combinations simpler to support, we should.

There's another problem here. Something that was overlooked here is that the type of the SpVType_shm_csr_matrix function parameter for these functions would need to become a template parameter since it is dependent on the SPValueType. Following that we could use the CSR_matrix_type::value_type to determine if we're dealing with complex or not.

To Ye's suggestion then, we would still need to make explicit instantiations for just the two sp value types (where the rest of the specialization of csr_matrix remains the same: int, std::size_t, shared_allocator).

* update is_complex template tree to look for generic complex * ask for is_complex_v instead of is_complex::value * macro instantiate template functions to help avoid linking errors

quantumsteve · 2023-06-03T02:02:11Z

src/type_traits/QMCMacros.h

+  QMC_FOREACH_REAL_TYPE(MACRO)  \
+  QMC_FOREACH_COMPLEX_TYPE(MACRO)


We may want to pick one the other based on QMC_COMPLEX.

if we're going for a unified build, shouldnt both be available?

ye-luo · 2023-06-05T19:50:01Z

src/AFQMC/Hamiltonians/HSPotential_Helpers.h

@@ -23,15 +23,17 @@ namespace afqmc
 {
 namespace HamHelper
 {
+template<class T>


I think making template<bool IS_COMPLEX> reduce the complexity of explicit template instantiation cases.

quantumsteve · 2023-06-08T16:53:46Z

src/AFQMC/config.0.h

+template <class T>
+struct is_complex : std::false_type {};
+
+template <class T>
+struct is_complex<std::complex<T>> : std::true_type {};


is_complex is already in the codebase

qmcpack/src/type_traits/complex_help.hpp

Line 17 in 2db0b25

template<typename T>

Naming convention is also wrong here.

PDoakORNL

I think this can be made a bit simpler and more readable by reexamining the typealias choices made in config.h.

csr_matrix lacks meaningful documentation but pretty clearly is primarily templated on a VALUE type

PDoakORNL · 2023-06-15T20:25:27Z

src/AFQMC/config.0.h

+template <class T>
+struct is_complex : std::false_type {};
+
+template <class T>
+struct is_complex<std::complex<T>> : std::true_type {};


Naming convention is also wrong here.

PDoakORNL · 2023-06-15T21:07:06Z

src/AFQMC/Hamiltonians/HSPotential_Helpers.h

 std::vector<std::size_t> count_nnz_per_cholvec(double cut, TaskGroup_& TG, SpVType_shm_csr_matrix& V2, int NMO);

+template<class T>


in config.h replace

using SpCType_shm_csr_matrix = ma::sparse::csr_matrix<SPComplexType, int, std::size_t, shared_allocator<SPComplexType>, ma::sparse::is_root>; using SpVType_shm_csr_matrix = ma::sparse::csr_matrix<SPValueType, int, std::size_t, shared_allocator<SPValueType>, ma::sparse::is_root>; using CType_shm_csr_matrix = ma::sparse::csr_matrix<ComplexType, int, std::size_t, shared_allocator<ComplexType>, ma::sparse::is_root>; using VType_shm_csr_matrix = ma::sparse::csr_matrix<ValueType, int, std::size_t, shared_allocator<ValueType>, ma::sparse::is_root>;

with

template<typename T> using CSRMtrxShared<T> = ma::sparse::csr_matrix<SPComplexType, int, std::size_t, shared_allocator<SPComplexType>, ma::sparse::is_root>;

In most cases this will just read
CSRMatrixShared<Value> or CSRMatrixShared<Complex> which is much easier to read quickly and surfaces the important parameter for both read and specializing code.

then one can write

template<typename T> ...(... CSRMatrix<T> WorthlessTwoLetterVariableName, ...) { .... if constexpr (IsComplex_t<T>::value) { ... } ... }

PDoakORNL · 2023-06-15T21:15:41Z

src/AFQMC/Hamiltonians/HSPotential_Helpers.cpp

+        assert(map_[2 * (*ci) + 1] >= 0);
+        assert(map_[2 * (*ci) + 1] - c_origin < vn.size(1));
+        vn.emplace_back({ik, (map_[2 * (*ci) + 1] - c_origin)},
+                        static_cast<SPValueType>(half * im * (*vi - ma::conj(*vi)))); // Lii - Lii*


this should be static_cast<decltype(vn)::value_type>

* use existing is_complex evaluators * add templated CSR Matrix * extend IsComplex evaluators with valued version

walshmm marked this pull request as ready for review June 2, 2023 17:50

walshmm changed the title ~~DRAFT: HSPotential Helper replace ifdefs with const expr templates~~ HSPotential Helper replace ifdefs with const expr templates Jun 2, 2023

prckent previously approved these changes Jun 2, 2023

View reviewed changes

quantumsteve reviewed Jun 2, 2023

View reviewed changes

walshmm dismissed prckent’s stale review via ebc93c0 June 2, 2023 20:15

walshmm added 2 commits June 2, 2023 16:16

replace ifdefs with const expr templates

a6abba9

* pull William's qmcmacros

4fdd6d2

* update is_complex template tree to look for generic complex * ask for is_complex_v instead of is_complex::value * macro instantiate template functions to help avoid linking errors

walshmm force-pushed the hspotentialhelper_complex-real_refactor branch from ebc93c0 to 4fdd6d2 Compare June 2, 2023 20:16

quantumsteve reviewed Jun 3, 2023

View reviewed changes

ye-luo reviewed Jun 5, 2023

View reviewed changes

quantumsteve reviewed Jun 8, 2023

View reviewed changes

PDoakORNL requested changes Jun 15, 2023

View reviewed changes

walshmm marked this pull request as draft June 23, 2023 15:14

update according to pr comments:

fb2f97d

* use existing is_complex evaluators * add templated CSR Matrix * extend IsComplex evaluators with valued version

walshmm closed this Nov 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HSPotential Helper replace ifdefs with const expr templates #4623

HSPotential Helper replace ifdefs with const expr templates #4623

walshmm commented Jun 2, 2023 •

edited

Loading

prckent left a comment

prckent commented Jun 2, 2023

quantumsteve Jun 2, 2023 •

edited

Loading

quantumsteve Jun 2, 2023

quantumsteve Jun 2, 2023

ye-luo Jun 2, 2023 •

edited

Loading

quantumsteve Jun 3, 2023

prckent Jun 3, 2023

PhilipFackler Jun 7, 2023

quantumsteve Jun 3, 2023

walshmm Jun 16, 2023

ye-luo Jun 5, 2023

quantumsteve Jun 8, 2023

PDoakORNL Jun 15, 2023

PDoakORNL left a comment

PDoakORNL Jun 15, 2023

PDoakORNL Jun 15, 2023

PDoakORNL Jun 15, 2023

	if constexpr (is_complex<T>::value)
	if constexpr (is_complex_v<T>)

		QMC_FOREACH_REAL_TYPE(MACRO) \
		QMC_FOREACH_COMPLEX_TYPE(MACRO)

		std::vector<std::size_t> count_nnz_per_cholvec(double cut, TaskGroup_& TG, SpVType_shm_csr_matrix& V2, int NMO);

		template<class T>

HSPotential Helper replace ifdefs with const expr templates #4623

HSPotential Helper replace ifdefs with const expr templates #4623

Conversation

walshmm commented Jun 2, 2023 • edited Loading

Proposed changes

What type(s) of changes does this code introduce?

Does this introduce a breaking change?

What systems has this change been tested on?

Checklist

prckent left a comment

Choose a reason for hiding this comment

prckent commented Jun 2, 2023

quantumsteve Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ye-luo Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PDoakORNL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

walshmm commented Jun 2, 2023 •

edited

Loading

quantumsteve Jun 2, 2023 •

edited

Loading

ye-luo Jun 2, 2023 •

edited

Loading