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

Interface for Yang-Mills gradient flow #1480

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

Conversation

aniketsen
Copy link
Contributor

Implemented an interface for Yang-Mills gradient flow (arXiv:1302.5246, Appendix D). Uses existing kernels for Wilson flow and Laplace operator. Added a function GFlowStep to perform individual sub-steps of the Wilson flow. For this purpose, moved WFlowStepType to enum_quda to make it globally accessible.

@aniketsen aniketsen requested a review from a team as a code owner July 9, 2024 22:54
@maddyscientist
Copy link
Member

@Jenkins ok to test

Copy link
Member

@maddyscientist maddyscientist left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution @aniketsen, an excellent addition to QUDA. I've left a few review comments, and a few things need fixed, but all should be easy to do.

Can you also apply clang-format to the diff please, as described here?

Do you have any thoughts on how we can correctness tests for this (as well as the regular Wilson Flow function) in QUDA? Short of fully implementing Wilson flow in a separate test code, is there anything that makes sense for testing?

extended, with the input field being exchanged prior to calling
this function. On exit from this routine, the output field will
have been exchanged.
@param[out] dataDs Output smeared field
Copy link
Member

Choose a reason for hiding this comment

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

I see there are typos in the doxygen here, inherited from the typos in WFlowStep above. Can you this these? E.g.,

     @param[out] dataDs Output smeared field

should be

     @param[out] out Output smeared field

GaugeField &gout = gaugeAux;

// helper gauge field for Laplace operator
GaugeField *precise = nullptr;
Copy link
Member

Choose a reason for hiding this comment

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

Now that the GaugeField class has copy and move constructors and assignment operators, there's no need to make this heap allocated (a pointer). Can you make this an object?

While there's plenty of legacy QUDA that uses pointers, going forward, new code should use stack based allocation where possible.

ColorSpinorParam cpuParam(h_in, *inv_param, gaugePrecise->X(), false, inv_param->input_location);
ColorSpinorField fin_h(cpuParam);

ColorSpinorParam cudaParam(cpuParam, *inv_param, QUDA_CUDA_FIELD_LOCATION);
Copy link
Member

Choose a reason for hiding this comment

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

Probably I prefer deviceParam or something else that doesn't state cuda in its name. Now that QUDA is portable, we should be saving the use of the CUDA word for target-specific things.

@aniketsen
Copy link
Contributor Author

@maddyscientist sorry for taking so long to get back. I have made the changes that you suggested.

For testing, we have a native cpu implementation which both the interfaces for Wilson flow and Gradient flow have been tested against. For a correctness test for Wilson flow, I don't see any other way than implementing the individual steps. For the Gradient flow, if the Wilson flow steps and ApplyLaplace are tested, the rest is just algebra, which I suppose is also separately tested.

@cpviolator
Copy link
Member

@maddyscientist @aniketsen That reminds me to implement some CPU code for (at least) the 3-staple action Stout and APE routines. I can get to that relatively soon if I find time away from my day job. Please leave it with me and I'll either make a separate PR or attach to this if it's not merged before I'm done.

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.

3 participants