-
Notifications
You must be signed in to change notification settings - Fork 0
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 templated class LCAOrbitalSetT #14
Add templated class LCAOrbitalSetT #14
Conversation
class LCAOrbitalSetT : public SPOSetT<T> | ||
{ | ||
public: | ||
using basis_type = SoaBasisSetBase<T>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming convention is caps for type i.e.
basis_type -> Basis
vgl_type -> VGL
....
IndexType -> Index
RealType -> Real
etc.
This is a change to have this more consistent and easier to read I think it should be taken.
using OffloadMWVArray = Array<T, 2, OffloadPinnedAllocator<T>>; // [walker, Orbs] | ||
|
||
///pointer to the basis set | ||
std::unique_ptr<basis_type> myBasisSet; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do have the UPtr
alias in src/type_traits/template_types
* SoA verson of LCOrtbitalSet | ||
* Localized basis set is always real | ||
*/ | ||
template<class T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a possibility that some of the arguments to SPOSet functions should always be FullPrec
So do we want to revisit that here? There should be a VALUE and VALUE_FP argument then.
I think here we should maintain the menomic trace that this is a Value type that is being templated on here. So use VALUE or VALUE_FP for the template parameter. Since there are rougly speaking two categories of templating on scalars in QMCPACK. There are scalars that historically are called values can be real/complex reduced/full and reals those that are real reduced/full.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PDoakORNL Would you repeat this question about SPOSetT<T>
on the main QMCPACK PR?
a9690c9
to
17e55cc
Compare
@quantumsteve please rebase the latest |
744c05f
to
df14b5a
Compare
Signed-off-by: Steven Hahn <[email protected]>
2874c4e
to
74c6706
Compare
Please review the developer documentation
on the wiki of this project that contains help and requirements.
Proposed changes
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
Does this introduce a breaking change?
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.