kcontainer now encapsulated and provides k elements at working prec #5389

PDoakORNL · 2025-03-20T17:24:14Z

Proposed changes

The important change is By providing accessors for working precision ksq and kcart in KContainer we continue to support low precision operations for mixed precision and full precision for full precision builds without having to branch at run time.

And KContainer now does its internal kshell sorting in full precision and previous PR's made the Lattice always full precision. This removes the ambiguity in number of kshells and which kpoints are members of which shells that mixed precision previously suffered from. This unblocks consistent unit testing that is dependent on kshell sorting. For instance for the Structure Factor estimator. Incidentally the number of kshells reported for mixed precision also becomes consistent for many other unit tests.

In order to have to compiler help direct member access that possibly returns incorrect precision KContainer made an actual class and accessors are also established for other members previously directly accessed.

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

Bugfix
Refactoring (no functional changes＿
Build related change）
Testing changes (e.g. new unit/integration/performance tests)
Documentation or build script changes
Other (please describe):

Does this introduce a breaking change?

Yes

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/No. 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
Yes/No. This PR adds tests to cover any new code, or to catch a bug that is being fixed
Yes/No. Documentation has been added (if appropriate)

prckent · 2025-03-20T19:02:41Z

Happily it appears that this will likely fix #5384 . For the two HEG test failures in the CI I would simply bump the test tolerance:

27/895 Test  #143: deterministic-heg_14_gamma-sj-batch-1-1-potential .................................................***Failed    0.07 sec
Tests for series 0
  Testing quantity: LocalPotential
    reference mean value     :  -2.02361764
    reference error bar      :   0.00002000
    computed  mean value     :  -2.02367767
    computed  error bar      :   0.00000000
    pass tolerance           :   0.00006000  (  3.00000000 sigma)
    deviation from reference :  -0.00006003  ( -3.00160500 sigma)
    error bar of deviation   :   0.00002000
    significance probability :   0.99731426  (gaussian statistics)
    status of this test      :   fail

prckent · 2025-03-20T23:00:57Z

Test this please

PDoakORNL · 2025-03-21T20:48:06Z

test this please

prckent

LGTM. (Will still need a non-ORNL merge).

Thanks Peter. I have a couple of minor points, but I don't think they are worth holding up the PR for: first, there is some commented code that would be nice to remove. second, the "Working" data might be more clearly named "WorkingPrec" , or at least a few more comments to this effect added.

ye-luo · 2025-03-24T17:21:23Z

src/Particle/LongRange/tests/test_StructFact.cpp


  for (int i = 0; i < ref.groups(); i++)
  {
-    std::complex<double> rhok_sum, rhok_even_sum;


My fix https://github.com/QMCPACK/qmcpack/pull/5382/files needs to be reinstated .

ye-luo · 2025-03-24T17:29:33Z

There are a lot of changes involved with replacing direct member access with accessor functions. That makes reviewing the actual change difficult. Could you make a list of bullet points highlighting functional changes?
second questions, how much timings are changed in Coulomb terms in performance tests?

PDoakORNL · 2025-03-24T20:11:40Z

Timings should be practically unchanged the cost of initializing the cached low precision values of ksq and kcart is outside of the particle move loop. There is a slight memory cost in mixed precision for the lowered precision ksq and kcart vectors.

PDoakORNL · 2025-03-25T17:14:38Z

test this please

ye-luo · 2025-04-01T19:46:42Z

src/Utilities/tests/for_testing/test_checkVector.cpp

 //////////////////////////////////////////////////////////////////////////////////////

 #include "catch.hpp"
+#include "OhmmsPETE/TinyVector.h"


I don't feel it is needed here. Can it be removed?

This one can't be removed. Either it has to be here or in checkVector.hpp and I remember a review comment removing it from that file.

ye-luo

Please make a separate PR with all the changes under Utilities. It should simplify the review process.

PDoakORNL · 2025-04-01T22:03:17Z

this is going to have a clash with #5410 which I need to look at after that merges so don't merge.

ye-luo · 2025-04-02T05:47:54Z

src/Particle/LongRange/KContainer.h

+  using PositionFull       = QMCTraits::QTFull::PosType;
+  static constexpr int DIM = OHMMS_DIM;
+  using AppPosition        = typename QMCTraits::PosType;
+  using Position           = typename QMCTypes<Real, DIM>::PosType;


With this change, there are three precision in this class.
REAL, QMCTraits::FullPrecRealType and QMCTraits::PosType (effectively QMCTraits::RealType)
This seems defeating the purpose of templates.

ye-luo · 2025-04-02T06:04:03Z

Trying a simple fix #5411
If it works, we can avoid convoluted precisions in KContainer.
I still think making KContainer template useful. It affects StructFact which does benefit from lower precision. Using template allows us enabling precision selection at runtime although this feature is considered low priority.

There are a lot of changes in this PR making KContainer comply to our coding standard. That is a very good thing to do. Unfortunately they made code review very hard. I'd recommend making such types of change separately from actual functional changes. It is unclear to me if we can still cherry-pick them or just redo them from scratch.

PDoakORNL · 2025-04-02T14:15:29Z

I think you are being unecessarily restrictive with this and having just had to write an midterm EPM I am accutely aware of how much my performance is effected by our code review process. @prckent please resolve this I am not initerested in revising this again and I do not think it is excessively difficult to review.

ye-luo · 2025-04-02T14:30:53Z

I did a very careful review of the whole code and I expressed my concern #5389 (comment) of the key functional change in this PR. Regarding your KPI, that is an offline discussion. If all the assessor changes were in a separate PRs, such PRs should have been merged already.

prckent · 2025-04-02T14:50:11Z

I'll take a proper look this afternoon. Dev velocity is a key point and also a concern of mine. I tend to think we should merge this since the tests pass and it is clearly a step in the right direction. No need to keep chopping it up.

ye-luo · 2025-04-02T14:56:25Z

I'll take a proper look this afternoon. Dev velocity is a key point and also a concern of mine. I tend to think we should merge this since the tests pass and it is clearly a step in the right direction. No need to keep chopping it up.

I don't advice chopping it up. Instead, just drop this PR and add accessor/private in the develop. Probably just need some scripting. That seems sufficient to get other dependent PRs to proceed.

Dev speed is critical. I frequently reorganized my intended changes into smaller PRs. Please please experienced developers do the same. It is not just helping reviewers but also to get their work finalized timely. They are the largest beneficiary.

prckent · 2025-04-02T21:38:19Z

Having reviewed the status, I don’t think the return on investment is there to justify splitting this PR or redoing it again (4th time?). While we all strive for small PRs, that doesn’t always work out. The current PR is clearly several steps forward and the tests pass. I think it is time to merge and move on to other things that will benefit QMCPACK users more (observables, shared spline memory etc.).

ye-luo · 2025-04-02T23:08:06Z

Let me explain my technical assessment of this PR. I raised the concern, the key functional change of this PR is flawed and made the code further complicated. To address this flaw, extra effort will be needed to clean up the over complicated precision handling if we merge this PR and I'm afraid it's not worth it. The alternative route will be actually using an easily understandable fix of the k sorting. Then do minimal fix (adding a few access functions) to enable structure factor estimator #5303/#5406. They are quite self contained. This alternative route has one drawback that KContainer is not made template. Basically we need to defer the template change.

My understanding that the urgent work is to enable estimators not making KContainers template. Taking the alternative route allows to make the needed fix and we can focus working on the actual needed code.

prckent

I am happy to merge this as is. Let me know your preference @PDoakORNL .

ye-luo

For expediting the development of the structure factor estimator, let us merge this PR.

ye-luo · 2025-04-03T21:53:01Z

Test this please

PDoakORNL mentioned this pull request Mar 20, 2025

fix mixed precision kpoint sorting #5327

Closed

PDoakORNL added 4 commits March 21, 2025 11:57

kcontainer now encapsulated and provides k elements at working prec

6e440ad

update heg mixed reference values now closer to full prec

ccb6756

documentation and formatting consistent full/mixed prec KContainer

ed773a3

Revert change from Lattice, always test both precisions KContainer

17f0313

PDoakORNL force-pushed the kcontainer_working_precision branch from fc69d34 to 17f0313 Compare March 21, 2025 15:58

PDoakORNL requested a review from ye-luo March 21, 2025 15:59

PDoakORNL changed the title ~~[WIP] kcontainer now encapsulated and provides k elements at working prec~~ kcontainer now encapsulated and provides k elements at working prec Mar 21, 2025

PDoakORNL changed the title ~~kcontainer now encapsulated and provides k elements at working prec~~ [WIP] kcontainer now encapsulated and provides k elements at working prec Mar 21, 2025

follow through the simplication of Lattice always beging full prec

1505dad

PDoakORNL changed the title ~~[WIP] kcontainer now encapsulated and provides k elements at working prec~~ kcontainer now encapsulated and provides k elements at working prec Mar 21, 2025

prckent previously approved these changes Mar 24, 2025

View reviewed changes

ye-luo reviewed Mar 24, 2025

View reviewed changes

return 5382 fix

02e0d5c

PDoakORNL dismissed prckent’s stale review via 02e0d5c March 24, 2025 19:55

CI failure resulted in hard look at SimpleParser

5d16623

prckent mentioned this pull request Apr 1, 2025

Replace NULL with nullptr in test_parser.cpp #5408

Closed

ye-luo reviewed Apr 1, 2025

View reviewed changes

remove no longer needed include

0c0d197

PDoakORNL added a commit to PDoakORNL/qmcpack that referenced this pull request Apr 1, 2025

synthetic PR from QMCPACK#5389 just get enhanced checkVector

254258b

ye-luo reviewed Apr 2, 2025

View reviewed changes

ye-luo mentioned this pull request Apr 2, 2025

Change Kshell sorting precision to full precision #5411

Draft

Merge branch 'develop' into kcontainer_working_precision

e629d04

prckent approved these changes Apr 3, 2025

View reviewed changes

ye-luo approved these changes Apr 3, 2025

View reviewed changes

Merge branch 'develop' into kcontainer_working_precision

55c4f1c

prckent merged commit 5bbbec5 into QMCPACK:develop Apr 4, 2025
41 checks passed

ye-luo mentioned this pull request Apr 6, 2025

Reinstall #5382 (deterministic-diamondC_1x1x1_pp-multi_dmc_sdj-1-1-2-mpc related fixes) #5424

Merged

prckent mentioned this pull request Apr 7, 2025

Long range unit test failures #5384

Closed

kcontainer now encapsulated and provides k elements at working prec #5389

kcontainer now encapsulated and provides k elements at working prec #5389

Uh oh!

Conversation

PDoakORNL commented Mar 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

prckent commented Mar 20, 2025

Uh oh!

prckent commented Mar 20, 2025

Uh oh!

PDoakORNL commented Mar 21, 2025

Uh oh!

prckent left a comment

Choose a reason for hiding this comment

Uh oh!

ye-luo Mar 24, 2025

Choose a reason for hiding this comment

Uh oh!

ye-luo commented Mar 24, 2025

Uh oh!

PDoakORNL commented Mar 24, 2025

Uh oh!

PDoakORNL commented Mar 25, 2025

Uh oh!

ye-luo Apr 1, 2025

Choose a reason for hiding this comment

Uh oh!

PDoakORNL Apr 1, 2025

Choose a reason for hiding this comment

Uh oh!

ye-luo left a comment

Choose a reason for hiding this comment

Uh oh!

PDoakORNL commented Apr 1, 2025

Uh oh!

ye-luo Apr 2, 2025

Choose a reason for hiding this comment

Uh oh!

ye-luo commented Apr 2, 2025

Uh oh!

PDoakORNL commented Apr 2, 2025

Uh oh!

ye-luo commented Apr 2, 2025

Uh oh!

prckent commented Apr 2, 2025

Uh oh!

ye-luo commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prckent commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ye-luo commented Apr 2, 2025

Uh oh!

prckent left a comment

Choose a reason for hiding this comment

Uh oh!

ye-luo left a comment

Choose a reason for hiding this comment

Uh oh!

ye-luo commented Apr 3, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

PDoakORNL commented Mar 20, 2025 •

edited

Loading

ye-luo commented Apr 2, 2025 •

edited

Loading

prckent commented Apr 2, 2025 •

edited

Loading