Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add LCAO mw_evaluateValue and mw_evaluateDetRatios #4611

Merged
merged 12 commits into from
Jun 24, 2023

Conversation

kgasperich
Copy link
Contributor

@kgasperich kgasperich commented May 23, 2023

Proposed changes

Implement LCAO::mw_evaluateValue using gemm.
Implement LCAO::mw_evaluateDetRatios

Address partly #4615

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

  • New feature

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/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)

GEMM impl for LCAOrbitalSet::mw_evaluateValue

loop over walkers impl for SPOSet::mw_evaluateValue and
SoaLocalizedBasisSet::mw_evaluateV

modified existing unit test to check output of
LCAOrbitalSet::mw_evaluateValue (also calls
SoaLocalizedBasisSet::mw_evaluateV)
@shivupa shivupa self-assigned this May 23, 2023
@shivupa shivupa requested a review from amandadumi May 23, 2023 22:18
@shivupa shivupa removed the request for review from amandadumi May 23, 2023 22:18
Copy link
Contributor

@ye-luo ye-luo left a comment

Choose a reason for hiding this comment

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

Please retitle this PR to reflect the current implementation. Prefer more frequent PRs.

int iat,
const RefVector<ValueVector>& psi_v_list) const
{
OffloadMWVArray phi_v;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this to shared resource.

OffloadMWVArray& psi_v) const
{
const size_t nw = spo_list.size();
OffloadMWVArray phi_v;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this to shared resource.

const std::vector<const ValueType*>& invRow_ptr_list,
std::vector<std::vector<ValueType>>& ratios_list) const
{
size_t nw = spo_list.size();
Copy link
Contributor

Choose a reason for hiding this comment

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

Add const

spo_list[iw].evaluateValue(vp_list[iw], iat, psi_list[iw]);
ratios_list[iw][iat] = simd::dot(psi_list[iw].get().data(), invRow_ptr_list[iw], psi_list[iw].get().size());
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The double loop works for now but eventually it needs to be modified to be offload friendly.

OffloadMWVArray& v)
{
for (size_t iw = 0; iw < P_list.size(); iw++)
evaluateV(P_list[iw], iat, v.data_at(iw,0));
Copy link
Contributor

Choose a reason for hiding this comment

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

clang-format?

size_t nw = psi_list.size();
SPOSet::ValueVector psi_v_1(n_mo);
SPOSet::ValueVector psi_v_2(n_mo);
RefVector<SPOSet::ValueVector> psi_v_list = {psi_v_1, psi_v_2};
Copy link
Contributor

Choose a reason for hiding this comment

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

RefVector<SPOSet::ValueVector> psi_v_list{psi_v_1, psi_v_2};

@prckent
Copy link
Contributor

prckent commented Jun 1, 2023

Echoing Ye's comment, we have learned that greater numbers of smaller PRs work best. Once the code presently here is satisfactory, I suggest we merge it and subsequent functionality can come in one or two functions at a time. An additional advantage of this is that the code will be running in the nightlies.

@kgasperich
Copy link
Contributor Author

@ye-luo I finally had a chance to get back to this, thanks for the feedback.
If the goal is small PRs then I think this might be good to go now.

@ye-luo
Copy link
Contributor

ye-luo commented Jun 23, 2023

Test this please

@ye-luo ye-luo changed the title [WIP] Offload lcao Add LCAO mw_evaluateValue and mw_evaluateDetRatios Jun 23, 2023
Copy link
Contributor

@ye-luo ye-luo left a comment

Choose a reason for hiding this comment

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

LGTM still need non-ANL approval

@ye-luo ye-luo marked this pull request as ready for review June 24, 2023 15:02
@ye-luo
Copy link
Contributor

ye-luo commented Jun 24, 2023

Test this please

@prckent
Copy link
Contributor

prckent commented Jun 24, 2023

Side question: Is the Identity specialization worth it? Is there a common case where it would be used?

@prckent
Copy link
Contributor

prckent commented Jun 24, 2023

Thanks Kevin

@prckent prckent merged commit a6a3275 into QMCPACK:develop Jun 24, 2023
21 checks passed
@prckent prckent mentioned this pull request Aug 18, 2023
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.

5 participants