Implement SPOSetT<T> template class hierarchy #4684

williamfgc · 2023-07-24T19:21:08Z

Proposed changes

Introduce a shim template class to asses the effort to change SPOSet to SPOSet<T>.
SPOSetT<T> currently does not have consumers nor tests.
Testing compilation in CI and define concrete types for the template classes.
The challenge is to adapt SPOSetT<T> to SPOSet current consumers.
Related to #4099

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?

sulfur

Checklist

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

williamfgc · 2023-07-24T20:49:54Z

Changed to the option suggested by @ye-luo to only concretize class type.

ye-luo · 2023-08-24T16:58:39Z

Thinking of how to avoid adding new files. Can we make SPOSetT<T> as an actual shim layer derived from SPOSet. Once a derived class is ported. It changes base from SPOSet to SPOSetT<T>. In this way, we don't add new files except SPOSetT<T>.

williamfgc · 2023-08-24T18:26:09Z

@ye-luo thanks for chiming in. We just thought having a separate SPOSetT hierarchy would help us with CI (at least build errors we introduce are caught) and for us to work in parallel. Once we have that we can start integrating with current SPOSet consumers without rewriting those and once done remove SPOSet and rename SPOSetT->SPOSet. Thoughts?

ye-luo · 2023-08-24T18:32:48Z

@ye-luo thanks for chiming in. We just thought having a separate SPOSetT hierarchy would help us with CI (at least build errors we introduce are caught) and for us to work in parallel. Once we have that we can start integrating with current SPOSet consumers without rewriting those and once done remove SPOSet and rename SPOSetT->SPOSet. Thoughts?

probably I didn't fully understand the current status of this PR. I saw multiple files (SPOSetT and derived classes) got added and was thinking of how to make the change mergeable without the need of porting every SPOSet derived classes before merging. By making SPOSet a base of SPOSetT probably allows us to keep ported and not yet ported classes to co-exist without any duplication. All the high level classes still just see SPOSet.

williamfgc · 2023-10-24T21:01:18Z

src/Particle/RealSpacePositionsTOMPTarget.h

@@ -178,6 +177,7 @@ class RealSpacePositionsTOMPTarget : public DynamicCoordinatesT<T>
        const size_t mw_pos_stride = mw_new_pos.capacity();

        PRAGMA_OFFLOAD("omp target teams distribute parallel for \
+                    is_device_ptr(mw_pos_ptr, mw_rosa_ptr) \


@ye-luo @correaa FYI this passes the right values

markdewing · 2023-10-26T16:55:05Z

src/QMCWaveFunctions/tests/test_RotatedSPOsT.cpp

+//
+// Copyright (c) 2022 QMCPACK developers.
+//
+// File developed by: Joshua Townsend, [email protected], Sandia National


It looks like someone is running clang-format with a column limit of 80 (vs the qmcpack default of 120). (Or is using an editor that does auto-wrapping). While technically within the guidelines (line length is < 120), it is creating extra differences between files.

markdewing · 2023-10-26T17:16:03Z

It looks like the "new" files have been run through clang-format that is more restrictive than the default one, or been modified in an editor that does some formatting.
What I've noticed so far:

Copyright header gets wrapped to 80 columns
Comments in general get wrapped to 80 columns
Space gets added between comment and text

I think it would be better if the copyright headers had a consistent format in every file.

Unfortunately, these aren't new files - they're renamed and changed versions of existing files.
The other changes wouldn't be a problem in a small PR, but for one this large, every change is making it harder for git to detect moved files (It uses some threshold for the number of pairs of changes to check, or a percentage of file similarity).

quantumsteve · 2023-10-26T17:45:07Z

cmake -G Ninja -DQMC_MPI=OFF ../

develop

$ time ninja
[646/646] Linking CXX executable bin/ppconvert

real	1m55.924s
user	64m36.053s
sys	5m4.636s

ref-add-SPOSetT

$ time ninja
[652/652] Linking CXX executable bin/ppconvert

real	2m5.956s
user	70m52.674s
sys	5m15.397s

markdewing · 2023-10-26T18:42:54Z

src/QMCWaveFunctions/tests/test_RotatedSPOsT.cpp

+template<typename T>
+struct ValueApproxHelper
+{
+  using Type = Catch::Detail::Approx;
+};
+template<typename T>
+struct ValueApproxHelper<std::complex<T>>
+{
+  using Type = Catch::Detail::ComplexApprox;
+};
+
+template<typename T>
+using ValueApprox = typename ValueApproxHelper<T>::Type;


This code might be useful elsewhere, and could get moved to src/external_codes/complex_approx.hpp or src/Configuration.h
(Not necessary for this PR, though)

williamfgc · 2023-10-26T23:27:26Z

@markdewing thanks for the catch. The latest commit should have addressed clang-format related issues. I will double-check. I used clang-format-16. Thanks!

williamfgc · 2023-10-27T01:13:03Z

Test this please

williamfgc · 2023-10-27T13:23:19Z

The changes in the Numerics and Particle directories clearly can't depend on SPOSet. And a lot of the changes in QMCDrivers are just minor changes like include files.

A lot of these changes are at the SPOSet<T> consumer level (e.g. builders) and likely 'cause GitHub Actions CI was breaking as at some point we ran it in every commit the current branch has. It's probably the smallest PR we can get with such refactoring.

williamfgc · 2023-10-27T14:52:28Z

Test this please

PDoakORNL · 2023-10-27T23:34:19Z

What's going on with the MCWalkerConfiguration further bleeding into the batched drivers and mw is undesirable. I've gone out of my way to keep this from happening. We want to drop that class when the legacy drivers and estimators go away. It should never be a source for type information for non legacy code.

The explicit definition of MCPWalker from Walker instead of pulling in Walker_t from elsewhere was intended to decouple classes that were tightly bound via Walker type aliases. Then they can be built and tested without creating cascading rebuilds. There seems to be a clash now between QMCTraits and ParticleTraits which is making a bit of a mess that is just getting fixed by adding includes that shouldn't really be included.

Similar issues seem behind the many forward declarations that were successfully decoupling headers that have been reverted by this PR.

I'm not sure if we should put this in and then work to refactor away the new includes and MCPWalker type aliases or just stage this in such a way we don't add all these design regressions.

prckent

I assent to merge this subject to the conflicts being fixed and there not being any new issues there.

If we are concerned about possible offload errors, we could check this code is OK on Frontier and Aurora, so that we don't risk breaking develop for FOM runs. Our CI coverage of offload is not great (only NV, limited coverage).

Our earlier decision was that this would be a large PR instead of a lot of small ones since the large PR route was much more practical. As Peter said, we can work to refactor away some of the new design dependencies. Breaking this PR up would not be consistent with what we agreed on, and from a project perspective there simply is not time to rework this PR. There are very few weeks remaining in the year to get unified complex-real builds completed.

Next step: William & co to fix the conflicts.

.github/workflows/ci-github-actions.yaml

williamfgc · 2023-11-08T01:06:22Z

Test this please

ye-luo

First, I appreciated the effort. It is a very heavy workload.
After looking through the change. I feel two aspects are mixed when rushing to make the code compile and run.

real/complex. This is what we would like to explore.
full/mixed precision. This is what I would like to leave aside.
by looking at all the changes. I'm surprised to see ParticleSet being template as ValueType. When making such massive changes. Certain choices may not be made optimal. Merging this PR as it is will be a challenge for post-ECP develops as there is very limited understanding of this PR and some modifications are not necessary and needs undo. So I still would like to explore a way to make piece wise changing possible.

I have additional thoughts about how to handle mixed precision and also simplify the code. It will be a separate discussion.

ye-luo · 2023-11-08T02:54:23Z

src/Particle/DistanceTable.h

-};
+using DistanceTable = DistanceTableT<QMCTraits::ValueType>;
+using DistanceTableAA = DistanceTableAAT<QMCTraits::ValueType>;
+using DistanceTableAB = DistanceTableABT<QMCTraits::ValueType>;


Distance tables are real only. Should not be in ValueType.

Yes. This needs to be fixed.

That can't be fixed without removing the member variable const ParticleSetT<T>& origin_; or making ParticleSetT<T> real-only.

qmcpack/src/Particle/DistanceTable.h

Line 51 in 864c22d

const ParticleSet& origin_;

Given that in the current state of develop DistanceTable depends tightly on ParticleSet and ValueType, I think it falls outside the scope of this PR.

ye-luo · 2023-11-08T03:25:19Z

src/Particle/ParticleSet.h

-    coordinates_->resize(numPtcl);
-  }
-};
+using ParticleSet = ParticleSetT<QMCTraits::ValueType>;


ParticleSet should be real-only.

Is MCWalkerConfiguration, which inherits from ParticleSet, real-only?

Since ParticleSet contains GradType and QMCTraits::GradType depends on ValueType. I don't see a straightforward way to make ParticleSet real-only.

qmcpack/src/type_traits/QMCTypes.h

Line 44 in 864c22d

using GradType = TinyVector<ValueType, DIM>;

Given that in the current state of develop ParticleSet depends on ValueType, I think it falls outside the scope of this PR.

williamfgc · 2023-11-08T12:14:11Z

full/mixed precision. This is what I would like to leave aside. by looking at all the changes. I'm surprised to see ParticleSet being template as ValueType.

This was done to satisfy the requirement to not specialize on a single function basis, but rather class level since ParticleSet is directly consumed by SPOSet and derived classes. If we start specializing it will look like lots of trailing #ifdef for handling mixed precision, and templates for real/complex. We think this is a good-enough compromise to move forward and not complicate current tasks given timelines. We didn't want to make complicated design changes (other than what we were asked) as we are relying on the current CI and many parts are not necessarily tested.

Merging this PR as it is will be a challenge for post-ECP develops as there is very limited understanding of this PR and some modifications are not necessary and needs undo. So I still would like to explore a way to make piece wise changing possible.

The PR itself is about undoing things, and there is plenty of pending things (e.g. we are still depending of #ifdef for template class definitions). I'm afraid any other attempt to make this piece wise won't add value and we we will end up with something very similar to what we have (or worse, combining current non-generic #ifdef patterns with template generic patterns). This is a good opportunity to identify pieces towards major re-architecture redesign, refactoring and add more testing. This PR can be summarized as SPOSet -> SPOSet <T> + the most straight-forward/shortest-path way to satisfy CI requirements. My concern is on trying to spend time to maximize impact given that resources are limited.

williamfgc · 2023-11-09T01:40:00Z

Test this please

Asses the effort to change this. Currently without consumers or tests. Concretize friend class declaration Define testing::getMyVars for SPOSetT Add FakeSPOT class Move SpinorSet to a templated class Refactor FreeOrbital class Base typed aliases on SPOSet<T> on OrbitalSetTraits<T> Add FullRealType in SPOSet and RotatedSPOs Add this in templated meta class Add explicit function instantions for FreeOrbital Add templated class SHOSetT Signed-off-by: Steven Hahn <[email protected]> Add PWRealOrbitalSetT template class Revert test_RotatedSPOs.cpp Revert test_RotatedSPOs.cpp Reorder specialized function definitions to appease OMP target compilation Add ConstantSPOSetT Refactor BsplineSet and SplineC2C Follow existing pattern for SplineC2C allowing for std::complex<T> PWOribitalSetT and PWBasisT Add FullRealType in SPOSet and RotatedSPOs Move generic definition after specialization add implicit implementations Fix some errors initial commit of templated PWOribitSetT that compiles cleanup templateitze PWBasis as well, as is dependancy remove inaccurate comment remove polluted commit Add LCAOrbitalSetT Add templated class LCAOrbitalSetWithCorrectionT Signed-off-by: Steven Hahn <[email protected]> Add SPOSetBuilderT, SHOSetBuilderT and SoaCuspCorrectionT Fix PWOrbitalSet alias types Fix LCAOrbitalSetWithCorrectionT Reuse SPOSet types Signed-off-by: Steven Hahn <[email protected]> Implement CompositeSPOSetT class Specialize functions in RotatedSPOsT Fix function signature Change order of definition Implement SPOSetBuilderFactoryT and most required builders Add missing implementation to SPOSetBuilderT Signed-off-by: Steven Hahn <[email protected]> Add CI to WIP refactoring branch add SplineR2RT Empty-Commit Further template propagation to fix offload build Implement SplineC2RTOMPTarget template class add missing particlesetT Refactored everything needed for test_RotatedSPOsT Add new bits to RotatedSPOsT Bugfix: removed QMC_COMPLEX conditions where no longer needed Accomodate new refactored headers simd::dot and Spline classes Move memory reference inside PRAGMA Causes a memory corruption when defining internal pos Fix pragma typo with is_device_ptr Implement SPOSetT template class Asses the initial effort to refactor SPOSet into templates without consumers or tests. Concretize friend class declaration Define testing::getMyVars for SPOSetT Add FakeSPOT class Move SpinorSet to a templated class Refactor FreeOrbital class Base typed aliases on SPOSet<T> on OrbitalSetTraits<T> Add FullRealType in SPOSet and RotatedSPOs Add this in templated meta class Add explicit function instantions for FreeOrbital Add templated class SHOSetT Add PWRealOrbitalSetT template class Revert test_RotatedSPOs.cpp Signed-off-by: Steven Hahn <[email protected]> Revert test_RotatedSPOs.cpp PWOribitalSetT and PWBasisT Add FullRealType in SPOSet and RotatedSPOs Move generic definition after specialization add implicit implementations Fix some errors initial commit of templated PWOribitSetT that compiles cleanup templateitze PWBasis as well, as is dependancy remove inaccurate comment remove polluted commit Start replacing legacy code with templated classes Fix ornl CI tests sulfur: Add missing SPOSet header in NiO tests nitrogen: Add missing memory header for std::unique_ptr clang-format-16 on new files Fix license file headers Fix typo with T1->T

Signed-off-by: Steven Hahn <[email protected]>

williamfgc · 2023-11-09T02:35:16Z

Test this please

williamfgc · 2023-11-09T14:27:56Z

@prckent @ye-luo @quantumsteve this is a new checkpoint that CI tests are passing and conflicts are resolved.

williamfgc requested review from ye-luo, PDoakORNL and prckent July 24, 2023 19:21

williamfgc force-pushed the ref-add-SPOSetT branch from 7306248 to c65db6a Compare July 24, 2023 20:49

williamfgc force-pushed the ref-add-SPOSetT branch from 6d3edef to 3449762 Compare July 25, 2023 14:49

williamfgc force-pushed the ref-add-SPOSetT branch from 2ddcc63 to 7dc04f6 Compare August 16, 2023 14:36

quantumsteve mentioned this pull request Aug 17, 2023

Add templated class LCAOrbitalSetT williamfgc/qmcpack#14

Merged

williamfgc force-pushed the ref-add-SPOSetT branch 2 times, most recently from 744c05f to df14b5a Compare August 22, 2023 18:49

quantumsteve mentioned this pull request Aug 24, 2023

SplineC2C/R2R rotation with BLAS #4710

Merged

williamfgc force-pushed the ref-add-SPOSetT branch 6 times, most recently from 9c61923 to eaae12d Compare August 28, 2023 20:32

quantumsteve mentioned this pull request Aug 31, 2023

Make setTwist/twist take/return a const reference. #4719

Merged

williamfgc force-pushed the ref-add-SPOSetT branch 3 times, most recently from d5f6b9b to c03543c Compare September 6, 2023 14:12

williamfgc force-pushed the ref-add-SPOSetT branch from bd5d50f to ca0baf4 Compare September 14, 2023 13:06

williamfgc force-pushed the ref-add-SPOSetT branch from 12cfc1f to 90194bc Compare September 28, 2023 13:28

williamfgc mentioned this pull request Oct 10, 2023

[WIP] Rebase ref-add-SPOSet on latest develop don't merge #4762

Closed

williamfgc force-pushed the ref-add-SPOSetT branch from 90194bc to 1359f2d Compare October 10, 2023 03:09

quantumsteve mentioned this pull request Oct 19, 2023

Remove unused template parameter #4786

Merged

williamfgc commented Oct 24, 2023

View reviewed changes

markdewing reviewed Oct 26, 2023

View reviewed changes

williamfgc force-pushed the ref-add-SPOSetT branch 3 times, most recently from f147c17 to 4712b2f Compare October 26, 2023 23:26

williamfgc force-pushed the ref-add-SPOSetT branch from 4712b2f to 3adc50f Compare October 27, 2023 14:51

williamfgc force-pushed the ref-add-SPOSetT branch from 3adc50f to a9c510b Compare October 27, 2023 15:05

prckent reviewed Nov 1, 2023

View reviewed changes

.github/workflows/ci-github-actions.yaml Outdated Show resolved Hide resolved

.github/workflows/ci-github-actions.yaml Outdated Show resolved Hide resolved

williamfgc force-pushed the ref-add-SPOSetT branch 3 times, most recently from dae4c3f to cc14aae Compare November 8, 2023 01:05

ye-luo reviewed Nov 8, 2023

View reviewed changes

williamfgc force-pushed the ref-add-SPOSetT branch from cc14aae to ac9fb92 Compare November 9, 2023 01:39

williamfgc and others added 4 commits November 8, 2023 20:49

Remove test differences and fix build

31a11f8

Signed-off-by: Steven Hahn <[email protected]>

Remove PR branch from CI

f919534

Fix missing const

c3298e8

williamfgc force-pushed the ref-add-SPOSetT branch from 630c02e to c3298e8 Compare November 9, 2023 01:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement SPOSetT<T> template class hierarchy #4684

Implement SPOSetT<T> template class hierarchy #4684

williamfgc commented Jul 24, 2023 •

edited

Loading

williamfgc commented Jul 24, 2023

ye-luo commented Aug 24, 2023

williamfgc commented Aug 24, 2023

ye-luo commented Aug 24, 2023

williamfgc Oct 24, 2023

markdewing Oct 26, 2023

markdewing commented Oct 26, 2023

quantumsteve commented Oct 26, 2023

markdewing Oct 26, 2023

williamfgc commented Oct 26, 2023 •

edited

Loading

williamfgc commented Oct 27, 2023

williamfgc commented Oct 27, 2023

williamfgc commented Oct 27, 2023

PDoakORNL commented Oct 27, 2023

prckent left a comment •

edited

Loading

williamfgc commented Nov 8, 2023

ye-luo left a comment

ye-luo Nov 8, 2023

prckent Nov 8, 2023

quantumsteve Nov 8, 2023 •

edited

Loading

williamfgc Nov 10, 2023 •

edited

Loading

ye-luo Nov 8, 2023

quantumsteve Nov 8, 2023

ye-luo Nov 8, 2023

quantumsteve Nov 8, 2023

williamfgc Nov 10, 2023 •

edited

Loading

williamfgc commented Nov 8, 2023 •

edited

Loading

williamfgc commented Nov 9, 2023

williamfgc commented Nov 9, 2023

williamfgc commented Nov 9, 2023

Implement SPOSetT<T> template class hierarchy #4684

Are you sure you want to change the base?

Implement SPOSetT<T> template class hierarchy #4684

Conversation

williamfgc commented Jul 24, 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

williamfgc commented Jul 24, 2023

ye-luo commented Aug 24, 2023

williamfgc commented Aug 24, 2023

ye-luo commented Aug 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markdewing commented Oct 26, 2023

quantumsteve commented Oct 26, 2023

Choose a reason for hiding this comment

williamfgc commented Oct 26, 2023 • edited Loading

williamfgc commented Oct 27, 2023

williamfgc commented Oct 27, 2023

williamfgc commented Oct 27, 2023

PDoakORNL commented Oct 27, 2023

prckent left a comment • edited Loading

Choose a reason for hiding this comment

williamfgc commented Nov 8, 2023

ye-luo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quantumsteve Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

williamfgc Nov 10, 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

williamfgc Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

williamfgc commented Nov 8, 2023 • edited Loading

williamfgc commented Nov 9, 2023

williamfgc commented Nov 9, 2023

williamfgc commented Nov 9, 2023

williamfgc commented Jul 24, 2023 •

edited

Loading

williamfgc commented Oct 26, 2023 •

edited

Loading

prckent left a comment •

edited

Loading

quantumsteve Nov 8, 2023 •

edited

Loading

williamfgc Nov 10, 2023 •

edited

Loading

williamfgc Nov 10, 2023 •

edited

Loading

williamfgc commented Nov 8, 2023 •

edited

Loading