Skip to content

Fix wrong output size in SplineC2ROMPTarget::mw_evaluateVGLandDetRatioGrads#4408

Merged
prckent merged 7 commits into
QMCPACK:developfrom
ye-luo:fix-wrong-size
Jan 23, 2023
Merged

Fix wrong output size in SplineC2ROMPTarget::mw_evaluateVGLandDetRatioGrads#4408
prckent merged 7 commits into
QMCPACK:developfrom
ye-luo:fix-wrong-size

Conversation

@ye-luo

@ye-luo ye-luo commented Jan 23, 2023

Copy link
Copy Markdown
Contributor

Proposed changes

There is bug when using offload real build.
This caused writing to undesired memory locations and wrong numbers (crazy high variance/energy >>1)

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

  • Bugfix
  • Testing changes (e.g. new unit/integration/performance tests)

Does this introduce a breaking change?

  • No

What systems has this change been tested on?

epyc-server

Checklist

  • Yes. This PR is up to date with current the current state of 'develop'

Fix SplineC2R. When the requested size is smaller than what can be supplied, only output the requested size.
What is the right way of selecting a subset of orbitals in an already built SplineC2R remains a big question.
The trucation is subject to the sorting of orbitals in C2R which puts all the "complex" orbitals first.
"complex" orbitals produce two real orbitals.
offload_scratch_iw_ptr + spline_padded_size * 4 + first,
spline_padded_size, first, index);
const size_t first_cplx = first / 2;
const size_t last_cplx = omptarget::min(last / 2, orb_size);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The min() here is wrong. orb_size refers to output real orbitals.

ValueType* restrict out_d2phi = out_dphi_z + phi_vgl_stride;

const size_t first_real = first_cplx + omptarget::min(nComplexBands_local, first_cplx);
const size_t last_real = last_cplx + omptarget::min(nComplexBands_local, last_cplx);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

last_real can be larger than requested_orb_size and caused overflow.

@ye-luo

ye-luo commented Jan 23, 2023

Copy link
Copy Markdown
Contributor Author

Test this please

@prckent
prckent self-requested a review January 23, 2023 14:42

@prckent prckent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks Ye. We discussed this a bit last week, but so we have a record here: why didn't asan catch this?

@ye-luo

ye-luo commented Jan 23, 2023

Copy link
Copy Markdown
Contributor Author

Thanks Ye. We discussed this a bit last week, but so we have a record here: why didn't asan catch this?

Because we don't have a test case exposing this bug. The added test triggers asan failure when I backported it to develop.

@ye-luo

ye-luo commented Jan 23, 2023

Copy link
Copy Markdown
Contributor Author

Test this please

@ye-luo

ye-luo commented Jan 23, 2023

Copy link
Copy Markdown
Contributor Author

I also noticed that asan runs on the github actions without QMC_DATA. We probably need to update the image to include NiO a4(S1) and a16(S4) h5 files.

@prckent prckent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Coverage would be higher but for #4409 .

We (I) should mention in the release notes that QMC_DATA and these NiO files are encouraged.

@prckent
prckent merged commit 2006024 into QMCPACK:develop Jan 23, 2023
@ye-luo
ye-luo deleted the fix-wrong-size branch February 13, 2023 23:54
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.

2 participants