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

templatizing SPOSet #5306

Merged
merged 1 commit into from
Feb 10, 2025
Merged

templatizing SPOSet #5306

merged 1 commit into from
Feb 10, 2025

Conversation

ylee88
Copy link
Contributor

@ylee88 ylee88 commented Feb 7, 2025

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

Proposed changes

Templatizing SPOSet to SPOSetT<T>. SPOSet is aliased to SPOSetT<QMCTraits::QTBase::ValueType>.

Ye's comment:
This is the beginning of an effort to allow a single build of QMCPACK with real and complex and also mixed precision.
We start from the lower level components of TWF and gradually turn the whole TWF into template.
This PR turns SPOSet template and both real and complex specialization are unconditionally instantiated. We will work on SPOSet derived classes next.

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

  • Refactoring (no functional changes, no api changes)

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

@ye-luo
Copy link
Contributor

ye-luo commented Feb 8, 2025

Test this please

Copy link
Contributor

@PDoakORNL PDoakORNL left a comment

Choose a reason for hiding this comment

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

Would like to see naming of the type aliases consistent with the coding standards and consistent with the aliases for the container types.

Otherwise this looks good to me.

@@ -576,6 +589,7 @@ class SPOSet : public QMCTraits
IndexType OrbitalSetSize;
};

using SPOSet = SPOSetT<QMCTraits::QTBase::ValueType>;
using SPOSetPtr = SPOSet*;
Copy link
Contributor

Choose a reason for hiding this comment

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

For some reason I have a reflexive dislike of the SPOSetT naming and this alias trick to reduce the code change ripple. But I am accepting it because I think the reduction of files to review and improved readability everywhere the underlying type of SPOSet doesn't matter makes up for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is temporary renaming to minimize code change for an easy review. Once we touch the consumers of SPOSet. SPOSetT will be renamed back to SPOSet.

DIM = OHMMS_DIM,
DIM_VGL = OHMMS_DIM + 2 // Value(1) + Gradients(OHMMS_DIM) + Laplacian(1)
};
using ValueType = T;
Copy link
Contributor

Choose a reason for hiding this comment

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

As always I am not a fan of the redundant xxxType naming of type aliases and types. I'm glad to see FullPrecRealType at least dropped this. But if the ripple isn't excessive can we drop the redundant Type suffix where ever possible. I have been trying to do it when I touch code where it is possible.

Copy link
Contributor

@ye-luo ye-luo Feb 10, 2025

Choose a reason for hiding this comment

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

ValueType cannot be changed due to the used in derived classes. That change requires a giant renaming PR which distract the intention of this PR to focus on adjust the base class only. When touching derived classes, Value can be introduced parallel to ValueType once we finished going through all derived classes, ValueType can be removed eventually.

@ye-luo ye-luo merged commit 0440704 into QMCPACK:develop Feb 10, 2025
41 of 43 checks passed
@prckent
Copy link
Contributor

prckent commented Feb 10, 2025

Q. Why did only SplineR2R.cpp need to be touched at this stage, and not (say) C2C?

@ye-luo
Copy link
Contributor

ye-luo commented Feb 10, 2025

Q. Why did only SplineR2R.cpp need to be touched at this stage, and not (say) C2C?

It is an inconsistency between R2R and C2C implementation.

Will PR to address that. Prefer changing the C2C side.

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.

4 participants