Added a STOUT link smearing routine.

[cpviolator]

I made a carbon copy of the APE smearing routines and replaced the link smearing with STOUT smearing.

[review-ninja]

[mathiaswagner]

Jenkins: Can one of the admins verify this patch?

[mathiaswagner]

Some remarks:
We use cmake now as the preferred build system. Thus we require the the CMakeLists.txt is updated for new features. We can help with that.

It seems like you forked QUDA into your own repository. We somehow prefer feature branches over forks. This allows all contributors to improve the code before merging the pull request. I am not sure whether I can modify e.g. the CMakeLists.txt in your branch.

If you started off from a carbon copy: Can we avoid the code duplication implied with that? Can you merge that all into one file?

For the testing: It is inherently bad to rely on external codes for testing. This means that only people with access can do these tests. Any chance that we can get a quda internal test?
Just afraid that otherwise we will end up with a code that nobody can modify because the original author is no longer an active contributor and the only who can make sure the routines are still correct.

Big warning: Not sure whether it is a github flaw but in the diff it looks like quda.h is completely new?
Or does this just happen for me? Just saw that this is a new file called ' include/quda.h:' (with a ':' at the end of the name) instead of ' include/quda.h'. Something is obviously wrong there and needs to be fixed.

[mathiaswagner]

Continuing with the interface:

 /**
     Apply APE smearing to the gauge field
     @param dataDs Output smeared field
     @param dataOr Input gauge field
     @param alpha smearing parameter
     @param location Location of the computation
  */
  void APEStep (GaugeField &dataDs,
        const GaugeField& dataOr,
        double alpha,
        QudaFieldLocation location);

  /**
     Apply STOUT smearing to the gauge field
     @param dataDs Output smeared field
     @param dataOr Input gauge field
     @param rho smearing parameter
     @param location Location of the computation
  */
  void APEStep (GaugeField &dataDs,
        const GaugeField& dataOr,
        double rho,
        QudaFieldLocation location);

The routine for stout should not be called APEStep.

[AlexVaq]

@mathiaswagner I agree we should rely on tools for internal testing, but this sometimes means a lot of development time we usually don't have, so I wouldn't be so hard in using external tools for testing, at least at the beginning.

It would be a good idea to merge the files for APE and stout, but it might not be straightforward. I'd go first with @cpviolator 's implementation and eventually revise the code. Since this is disconnected with the rest of QUDA, it won't be hard to merge afterwards.

[mathiaswagner]

@AlexVaq Don't get me wrong I do understand the issue but for long-term maintainability of QUDA having to rely on external tests is bad. And once the code is in it is even harder to force people to take care of tests (see #19)
QUDA has become an important piece of software and to ensure its maintainability we need tests.

There are codes that haven been written by people years ago and have left LQCD and if we don't have good comments and tests this effectively means that at some point we have to remove that feature or rewrite it from scratch because it is just not maintainable.

Just to motivate my intention.

[AlexVaq]

Oh yes, I'm aware of the problem. I wanted to get involved in #19, but I suppose you know the rest.

Kate did a pretty good job unifying the CPU and GPU franework, I'll have a look at that because I think it's the easiest way to go.

El 16 dic 2015, a las 13:42, Mathias Wagner [email protected] escribió:

@AlexVaq Don't get me wrong I do understand the issue but for long-term maintainability of QUDA having to rely on external tests is bad. And once the code is in it is even harder to force people to take care of tests (see #19)
QUDA has become an important piece of software and to ensure its maintainability we need tests.

There are codes that haven been written by people years ago and have left LQCD and if we don't have good comments and tests this effectively means that at some point we have to remove that feature or rewrite it from scratch because it is just not maintainable.

Just to motivate my intention.

—
Reply to this email directly or view it on GitHub.

[maddyscientist]

@mathiaswagner It looks like the CMakeList.txt has been update already. Or is something missing?

@cpviolator Apart from the issues pointed out above by @mathiaswagner (quda.h appearing as a new file, and incorrect header declaration) this looks like great work. Once you get these fixed, we should be ok to merge this. You can also update the README to add your name to the contributors. 😄

Regarding the lack of a test. This is fixable, we can do some heatbath updates, and do some Stout / APE smearing. If we record a reference plaquette this can be used to check for correctness. In general we are going to have to move to expanding our implicit tests else otherwise we will be reimplementing the entire library in the tests directory 😜

[mathiaswagner]

@maddyscientist 'lib/CMakeLists.txt' had been updated but the description of the option for the gauge tools option has only been updated for autoconf.

[maddyscientist]

@mathiaswagner Understood. Thanks.

[cpviolator]

@mathiaswagner Thank you for spotting those errors. Once you are all happy with the changes and accept the pull request, I will delete the my personal fork and use branching. Then I'll take a look at merging the STOUT and APE files into a single `gauge_smear.cu' file. Before too long I expect to have nHYP or HEX smearing code written for CPS, so further routines can be added.

[maddyscientist]

@cpviolator I've given you push access to the github lattice group, so you're a bonafide member of QUDA. 😉

[cpviolator]

@maddyscientist Thank you very much! (just in time for job interviews too!)

[maddyscientist]

@cpviolator Looks like you made all the changes you needed to do. It looks good.

It's probably lack of communication on our part, but the way we operate the pull request system is that once the pull request is issued, the pull request is reviewed by at least one other team member before it is merged (by the other team member). The motivation here is to keep the develop branch stable, and to make sure any issues are caught. So in general, who ever issues the pull request is not the person who should be hitting the merge button. No big deal here, just something to be aware of for the future.

Thanks again for implementing this so quickly. Did you manage to verify it again your CPS implementation?

[cpviolator]

My Apologies! I'm not exactly new to GitHub, but this is my first contribution to a collaborative project. I will in future be try to be more communicative and ask questions when I'm unsure rather than pull the trigger.

Alex sent me some code that he used to test his APE code which I can use for this stout build; I will do very soon. I agree with the discussion about making tests internal to QUDA so I will also look at what would be involved in writing that. The Wilson plaquette sum seems to be a fairly robust parameter, especially because, unlike a correlation function value, it is independent of any results that come from an inversion algorithm.

Thank you for all your encouragement, I'm very happy to be involved and I hope to make many more contributions.

[mathiaswagner]

My 'CMakeLists.txt' is missing :-(

Just wondering whether we should have a look a protected branches again. The administrative overhead should be close to zero.
Also, @cpviolator : There is not a lot of text but we roughly follow the gitflow model. There are some links on the quda wiki page. As a new contributor it's worth a read. Welcome to the team !

[cpviolator]

@mathiaswagner I'm sorry, but I don't understand. Was the 'CMakeLists.txt' file not included in the push? I did not edit it nor did I remove it. I wholly accept any blame associated with this error, but I don't know what I did wrong.

[mathiaswagner]

No worries: The description of the build option in the root level CMakeLists was just not updated (as you did for configure). Really just a tiny thing and I'll take care of this. I will do some bigger change there anyway.

[maddyscientist]

@mathiaswagner it looks like the description was already more informative in configure than in cmake anyway. I think it was easy to overlook this since all CMakelist.txt said was

set(BUILD_GAUGE_TOOLS OFF CACHE BOOL "build auxilary gauge-field tools")                                                      

whereas configure.ac said

AC_ARG_ENABLE(gauge-tools,
  AC_HELP_STRING([--enable-gauge-tools], [ Build auxilary gauge tools: plaquette, gauge evolver, APE, extended gauge routines (default: disabled)]),
  [ build_gauge_tools=${enableval} ],
  [ build_gauge_tools="no" ]
)

which was more suggestive for an update.

And I see in both cases auxiliary is spelt wrong 😛

[cpviolator]

Ahh, I see what happened. I grepped for 'APE' and 'ape' in the quda library and only made adjustments where I saw an instance of 'APE' or 'ape'. I see now that `build_gauge_tools' is a higher level of abstraction I should be looking for. Thanks for the warning.

[mathiaswagner]

My bad. Sorry.

On 17.12.2015, at 06:38, maddyscientist <[email protected]mailto:[email protected]> wrote:

@mathiaswagnerhttps://github.com/mathiaswagner it looks like the description was already more informative in configure than in cmake anyway. I think it was easy to overlook this since all CMakelist.txt said was

set(BUILD_GAUGE_TOOLS OFF CACHE BOOL "build auxilary gauge-field tools")

whereas configure.ac said

AC_ARG_ENABLE(gauge-tools,
AC_HELP_STRING([--enable-gauge-tools], [ Build auxilary gauge tools: plaquette, gauge evolver, APE, extended gauge routines (default: disabled)]),
[ build_gauge_tools=${enableval} ],
[ build_gauge_tools="no" ]
)

which was more suggestive for an update.

And I see in both cases auxiliary is spelt wrong [:stuck_out_tongue:]

Reply to this email directly or view it on GitHubhttps://github.com//pull/418#issuecomment-165342600.

NVIDIA GmbH, Wuerselen, Germany, Amtsgericht Aachen, HRB 8361
Managing Director: Karen Theresa Burns

---

This email message is for the sole use of the intended recipient(s) and may contain
confidential information. Any unauthorized review, use, disclosure or distribution
is prohibited. If you are not the intended recipient, please contact the sender by

reply email and destroy all copies of the original message.