1rdm with spinor support #4807

camelto2 · 2023-10-30T18:07:46Z

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

Proposed changes

This PR adds the spinor support into the 1rdm. If spinor=true in the particleset, it will generate spin moves similar to how the spatial moves are generated.

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

Delete the items that do not apply

New feature

Does this introduce a breaking change?

No

What systems has this change been tested on?

macOS

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. Code added or changed in the PR has been clang-formatted
Yes. 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)

This implementation just adds a bunch of if statements on whetehr to include spin gradients or not. Ideally, I would like to clean this up a lot and template it on MCCoords, but this should be adequate for now

PDoakORNL

New code requires test support

Many PR's are coming in with none, you should be writing tests as you develop not as an afterthought later.

jtkrogel · 2023-11-01T15:17:14Z

#4789 will have a test added prior to merge following the discussion on that PR.

For this one, I am firmly requesting a merge of functionality first, followed by an agreed upon and fulfilled PR including the test(s) by Cody.

The functionality here is part of an active CMS research project and it is important that this functionality makes it into develop and remains current there so that other bugs we are encountering (blocks to research) can be clearly identified in develop and fixed.

PDoakORNL

A big part of keeping code current is being able to tell whether changes made to the code break it or not. This requires tests. All that really needs to be done here is to look at the current testing and figure out how to add cases for pset_targets that are spinors. This probably requires adding a mock particle set factory function to Particle/tests/MinimalParticlePool.h, which makes me wonder how much serious testing any of the spinor code has had.

PDoakORNL · 2023-11-01T21:33:52Z

src/Estimators/OneBodyDensityMatrices.h

@@ -276,12 +300,16 @@ class OneBodyDensityMatrices : public OperatorEstBase

  //  basis set updates
  void updateBasis(const Position& r, ParticleSet& pset_target);
+  //  basis set updates with spin


use /// or /** ... */ otherwise method documentation won't be produced by doxygen.

addressed these

PDoakORNL · 2023-11-01T21:35:13Z

src/Estimators/OneBodyDensityMatrices.cpp

@@ -354,6 +361,82 @@ inline void OneBodyDensityMatrices::generateDensitySamples(bool save,
  rhocur_ = rho;
 }

+inline void OneBodyDensityMatrices::generateDensitySamplesWithSpin(bool save,


generateDensitySamples has a unit test this can as well.

PDoakORNL · 2023-11-01T21:37:31Z

src/Estimators/OneBodyDensityMatrices.h

@@ -234,11 +241,19 @@ class OneBodyDensityMatrices : public OperatorEstBase
   *   *
   */
  void generateDensitySamples(bool save, int steps, RandomBase<FullPrecReal>& rng, ParticleSet& pset_target);
+
+  //same as above, but with spin variables included


camelto2 · 2023-11-01T22:02:03Z

A big part of keeping code current is being able to tell whether changes made to the code break it or not. This requires tests. All that really needs to be done here is to look at the current testing and figure out how to add cases for pset_targets that are spinors. This probably requires adding a mock particle set factory function to Particle/tests/MinimalParticlePool.h, which makes me wonder how much serious testing any of the spinor code has had.

The spinor code has had plenty of testing. We didn't use minimal particle pool. Most of the tests build particle sets by hand, similar to how many tests in the code have been written in the past

PDoakORNL · 2023-11-01T22:13:01Z

This is probably high enough level to make switch over to the mock pools worth it. Which I think I agreed to help with in the past. And probably didn't which helped us get here. One requirement would be a very small wavefunction file.

prckent

The discussion here rightly points out that we need a written up policy that we can point to and apply uniformly.

There has been a lot of discussion in various parts of Slack about this.

The requirement I am going to recommend here and that we apply in the future is simply that there is at least testing of some flavor in this PR, i.e. >=1 test, that executes a significant (>50%) fraction of lines in the code. Test coverage should not decrease as a result of the PR. Many of the reasons for this have been discussed already, but importantly it would allow someone other than the original code author to make a change somewhat safely. The change might be something a simple as formatting or updating error handling. This level of testing also helps guard against memory leaks and corruption creeping in. The test could be a deterministic integration test or (preferred) a set of unit tests. As already discussed, setting up unit tests can be a pain for complex objects. It is the responsibility of the implementer to go through that pain however and not create subsequent pain for others -- we have plenty of historical lessons is this area, hence the vigorous discussion. The rest of us will be trying to minimize the pain to create unit tests by docs/tutorials/hackathons/better design. This should get easier with time since we are going to be deleting lots of legacy and refactoring further, which will disentangle more code. => So for the time being this PR needs a test added. Based on the discussion of working QMC runs, a deterministic test is straightforward and will not take much time. Subsequent PRs will need to add docs, examples etc., but I don't think this was ever in question.

camelto2 · 2023-11-02T23:31:06Z

I would like to make it clear that I absolutely understand the need for testing, and that I never intended to make a PR without a test. I only made a PR without a test because I was requested to do so. For most of my development tasks, I do start my code development with a unit test before doing any of the implementation. Sometimes, depending on what I'm doing, it is easier to just get something working and worry about a test later before trying to merge if I'm just trying to see how or if something would work. It may not be the best practice, but in this case that is what I did. I added the functionality quickly so that Jaron could start to play around with it without it being made a permanent feature in the code. I haven't had time to work on making the tests due to other constraints, so I was planning to just keep my branch up to date with develop and merge eventually whenever I actually got around to writing a test. However, I was asked to go ahead and make a PR now, so I did.

ye-luo

Could you also address the CI failure?

ye-luo · 2023-11-03T03:05:19Z

src/Estimators/OneBodyDensityMatrices.h

+  //spin related variables
+  Real spcur_;
+  Real dspcur_;
+  bool is_spinor_;


This can be made const.

jtkrogel · 2023-11-03T17:20:56Z

Yes, I requested the PR. Code development ideals and research needs are at odds, creating a tug-of-war over which of our own feet to shoot. Hopefully with some future discussion we can sufficiently reduce the productivity drains on both sides of this issue, but I expect it will take some effort and creativity.

prckent · 2023-11-17T19:05:11Z

(Going through open PRs and checking what is needed to get them merged)
It seems that the ingredients to get this merged are present but the PR has never passed in CI.

camelto2 · 2023-11-17T21:47:15Z

I'm having trouble reproducing some of the failures I'm seeing in some of the test. One of them is bus error, another a segfault. I haven't been able to reproduce those locally so I'm not sure how to go about debugging it

prckent · 2023-11-21T19:34:25Z

Have you tried to decode the error found by the asan complex build? This shows that there is a memory related error in the estimator unit test.

camelto2 · 2023-11-21T22:38:35Z

Have you tried to decode the error found by the asan complex build? This shows that there is a memory related error in the estimator unit test.

I think I have the fix for that now. I wasn't able to get that failure to trigger on my machines, but I think I tracked it down anyway.

There are still tests failing from "maybe unitinitialized" which I've been trying to fix. My simple fixes aren't getting rid of the warning, and it seems to be happening due to the -O2, -O3 optimization flags. When I build with -O1, I do not trigger that warning, so i'm still trying to sort that out

camelto2 · 2023-11-22T00:07:47Z

Have you tried to decode the error found by the asan complex build? This shows that there is a memory related error in the estimator unit test.

I think I have the fix for that now. I wasn't able to get that failure to trigger on my machines, but I think I tracked it down anyway.

There are still tests failing from "maybe unitinitialized" which I've been trying to fix. My simple fixes aren't getting rid of the warning, and it seems to be happening due to the -O2, -O3 optimization flags. When I build with -O1, I do not trigger that warning, so i'm still trying to sort that out

should all be fixed now

camelto2 · 2023-12-04T16:44:50Z

Pinging this again, I think this is now ready to go

ye-luo

LGTM. Ping @prckent @PDoakORNL

prckent

Moving in the right direction.

More generally, presumably you have also thought about the random sampling implemented here:
I do wonder if the random sampling is an unnecessary complication. Do we have data to justify? The dimensionality of the integral is not that large so random sampling won't auotmatically have a large benefit.

prckent

Moving in the right direction.

More generally, presumably you have also thought about the random sampling implemented here:
I do wonder if the random sampling is an unnecessary complication. Do we have data to justify? The dimensionality of the integral is not that large so random sampling won't automatically have a large benefit.

prckent · 2023-12-06T14:19:27Z

Test this please

ye-luo · 2024-03-04T20:47:07Z

src/Estimators/OneBodyDensityMatrices.h

@@ -145,6 +147,11 @@ class OneBodyDensityMatrices : public OperatorEstBase
  /// current density
  Real rhocur_ = 0.0;

+  ///spin related variables
+  Real spcur_;
+  Real dspcur_;


FYI. The above two uninitialized variables caused failure https://github.com/QMCPACK/qmcpack/actions/runs/8022503383/job/21922019273?pr=4928
Fixed by 1aa5148
Unfortunately compilers didn't warn us in this case.

camelto2 added 11 commits May 15, 2023 17:53

initial implementation

da41925

This implementation just adds a bunch of if statements on whetehr to include spin gradients or not. Ideally, I would like to clean this up a lot and template it on MCCoords, but this should be adequate for now

forgot switch in generateSamples

86737a8

add break

9531313

forgot to resize array in composite SPOSet

4427709

fix generateSampleRatios for spinor moves

edf4c8b

Merge remote-tracking branch 'upstream/develop' into 1rdm_spinor

a7103f3

Merge remote-tracking branch 'upstream/develop' into 1rdm_spinor

28482c4

Merge remote-tracking branch 'upstream/develop' into 1rdm_spinor

7498a3b

Merge remote-tracking branch 'upstream/develop' into 1rdm_spinor

22a03c6

Merge remote-tracking branch 'upstream/develop' into 1rdm_spinor

c1c8478

Merge remote-tracking branch 'upstream/develop' into 1rdm_spinor

89976db

PDoakORNL requested changes Nov 1, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/develop' into 1rdm_spinor

05f2dd6

prckent self-requested a review November 2, 2023 19:17

prckent requested changes Nov 2, 2023

View reviewed changes

ye-luo reviewed Nov 3, 2023

View reviewed changes

camelto2 added 7 commits November 8, 2023 17:21

Merge remote-tracking branch 'upstream/develop' into 1rdm_spinor

608009e

Merge remote-tracking branch 'upstream/develop' into 1rdm_spinor

2fbd83c

Merge branch 'QMCPACK:develop' into 1rdm_spinor

c7bb140

Merge remote-tracking branch 'upstream/develop' into 1rdm_spinor

e8bc465

Merge remote-tracking branch 'origin/1rdm_spinor' into 1rdm_spinor

2534c73

initial unit test, making sure 1RDM actually does something with spins

b765edb

guard spinor wave functions for complex

0b8aa2f

camelto2 added 2 commits November 17, 2023 14:02

fix const and address CI failure

0ed8cec

Merge remote-tracking branch 'upstream/develop' into 1rdm_spinor

f1fb128

camelto2 added 5 commits November 20, 2023 17:40

Merge remote-tracking branch 'upstream/develop' into 1rdm_spinor

1542176

Merge remote-tracking branch 'upstream/develop' into 1rdm_spinor

11e13a3

Merge remote-tracking branch 'origin/1rdm_spinor' into 1rdm_spinor

c537300

Merge remote-tracking branch 'upstream/develop' into 1rdm_spinor

362a42c

address comments

cc4dd10

camelto2 added 2 commits November 21, 2023 15:30

fix issue in compositeSPOSet

7f06e6f

Merge remote-tracking branch 'upstream/develop' into 1rdm_spinor

29db6c8

remove unnecessary intermediate variable

24d32d9

Merge branch 'develop' into 1rdm_spinor

b46d81e

ye-luo approved these changes Dec 5, 2023

View reviewed changes

camelto2 requested review from PDoakORNL and prckent December 6, 2023 01:04

prckent approved these changes Dec 6, 2023

View reviewed changes

Merge branch 'develop' into 1rdm_spinor

55483da

PDoakORNL approved these changes Dec 6, 2023

View reviewed changes

PDoakORNL merged commit b0faa12 into QMCPACK:develop Dec 6, 2023
21 checks passed

camelto2 deleted the 1rdm_spinor branch December 6, 2023 16:56

camelto2 mentioned this pull request Dec 8, 2023

Non-collinear 1RDM #4560

Open

4 tasks

ye-luo reviewed Mar 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1rdm with spinor support #4807

1rdm with spinor support #4807

camelto2 commented Oct 30, 2023 •

edited

Loading

PDoakORNL left a comment

jtkrogel commented Nov 1, 2023

PDoakORNL left a comment

PDoakORNL Nov 1, 2023

camelto2 Nov 21, 2023

PDoakORNL Nov 1, 2023

camelto2 Nov 27, 2023

PDoakORNL Nov 1, 2023

camelto2 Nov 27, 2023

camelto2 commented Nov 1, 2023

PDoakORNL commented Nov 1, 2023

prckent left a comment •

edited

Loading

camelto2 commented Nov 2, 2023 •

edited

Loading

ye-luo left a comment

ye-luo Nov 3, 2023

camelto2 Nov 17, 2023

jtkrogel commented Nov 3, 2023

prckent commented Nov 17, 2023

camelto2 commented Nov 17, 2023

prckent commented Nov 21, 2023

camelto2 commented Nov 21, 2023

camelto2 commented Nov 22, 2023

camelto2 commented Dec 4, 2023

ye-luo left a comment

prckent left a comment

prckent left a comment

prckent commented Dec 6, 2023

ye-luo Mar 4, 2024

1rdm with spinor support #4807

1rdm with spinor support #4807

Conversation

camelto2 commented Oct 30, 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

PDoakORNL left a comment

Choose a reason for hiding this comment

jtkrogel commented Nov 1, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camelto2 commented Nov 1, 2023

PDoakORNL commented Nov 1, 2023

prckent left a comment • edited Loading

Choose a reason for hiding this comment

camelto2 commented Nov 2, 2023 • edited Loading

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

jtkrogel commented Nov 3, 2023

prckent commented Nov 17, 2023

camelto2 commented Nov 17, 2023

prckent commented Nov 21, 2023

camelto2 commented Nov 21, 2023

camelto2 commented Nov 22, 2023

camelto2 commented Dec 4, 2023

ye-luo left a comment

Choose a reason for hiding this comment

prckent left a comment

Choose a reason for hiding this comment

prckent left a comment

Choose a reason for hiding this comment

prckent commented Dec 6, 2023

Choose a reason for hiding this comment

camelto2 commented Oct 30, 2023 •

edited

Loading

prckent left a comment •

edited

Loading

camelto2 commented Nov 2, 2023 •

edited

Loading