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

Cleaned up isWilson/isStaggered/isDwf/getStencilSteps routines #1488

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

weinbe2
Copy link
Contributor

@weinbe2 weinbe2 commented Aug 30, 2024

This PR addresses issue #1098 and extends it to the getStencilSteps function as well, replacing various functions that look like this:

bool isWilsonType() const
    {
      return (Type() == typeid(DiracWilson).name() || Type() == typeid(DiracWilsonPC).name()
              || Type() == typeid(DiracClover).name() || Type() == typeid(DiracCloverPC).name()
              || Type() == typeid(DiracCloverHasenbuschTwist).name()
              || Type() == typeid(DiracCloverHasenbuschTwistPC).name() || Type() == typeid(DiracTwistedMass).name()
              || Type() == typeid(DiracTwistedMassPC).name() || Type() == typeid(DiracTwistedClover).name()
              || Type() == typeid(DiracTwistedCloverPC).name()) ?
        true :
        false;
    }

And turning them into Dirac abstract functions so each individual class is "forced" to define it so functions like isWilsonType, etc, don't need to be manually updated each time (or, more realistically, don't accidentally get forgotten).

There are some other functions that could this be applied to, hermitian() and hasSpecialMG(), but it seemed reasonable to leave those as false unless proven otherwise.

This PR now replaces those above functions with one-stop shop static Dirac::[...] functions that can query if various QudaDslashType or QudaDiracType enums correspond to Wilson, dwf, or staggered operators. The various Dirac* classes then query these static functions as appropriate. There is scope to apply these functions elsewhere in QUDA.

This PR also fixes a small bug in split grid for staggered vs asqtad type fermions.

@@ -368,9 +383,9 @@ namespace quda {
QudaMatPCType getMatPCType() const { return matpcType; }

/**
@brief I have no idea what this does
@brief returns the number of stencil applications per dslash application; 1 for fused operators, generally 2 otherwise
Copy link
Member

Choose a reason for hiding this comment

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

I think this description is wrong. It's 1 for operator that consist of a single hopping term, generally full operators, and 2 for composite operators that consist of two hopping terms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's now true in practice, however there is an exception with the clover hasenbush operator where the "full" operator is still a separate D_eo and D_oe. Getting this counting correct was necessary for the worker abstraction in various operators.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll update the wording to make that clearer, though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/**
@brief return if the operator is a Wilson-type 4-d operator
*/
virtual bool isWilsonType() const = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Having every operator need to specify this seems a bit cumbersome. Why not default all of these operators to return false, and then specialize where needed? DiracWilson can return true and all children will then inherit that. Similarly for staggered and DWF.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was cumbersome for me, but it's a marginal effort for anyone going forward. The reason why I made it abstract and not have it return false by default is to force folks to think about it, but again only once.

I do like the idea of setting it to true for sufficiently high-up "base" classes such as DiracWilson, so I'll pop that in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@weinbe2 weinbe2 added the bug label Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants