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

Feature/openqxd #1414

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

Feature/openqxd #1414

wants to merge 160 commits into from

Conversation

chaoos
Copy link

@chaoos chaoos commented Nov 6, 2023

  • Adds interface openQxD <--> QUDA
  • Adds capability for C* boundaries in 1,2 or 3 spatial directions
  • Fixed CPU reordering
  • Adds openQCD gamma matrix basis (and basis transformations)
  • Adds order class for spinors
  • Adds order class for clover field (U(1) clover field is transfered from openQxD)
  • Adds order class for gauge field
  • Changed all the comments in quda.h to C89 style comments
  • Rebased on your current develop branch

mathiaswagner and others added 30 commits September 26, 2022 12:39
The rank formula was updated to the openQxD layout: rank = coords[0]*dims[1]*dims[2]*dims[3] + coords[1]*dims[2]*dims[3]  + coords[2]*dims[3] + coords[3]
The forloop was corrected (typo in i counter)
corrected to i<=3
trying to manually couple indexing correctly
Currently, the reordering is done inside OpenQxd
Probably it will continue to work for spinor fields... To be confirmed..
In check3, only half the sites are loaded onto quda (odd ones on whole lattice), then in the save function, only the odd ones in the first half get reloaded!
@chaoos chaoos requested review from a team as code owners December 15, 2023 17:48
@chaoos
Copy link
Author

chaoos commented Feb 27, 2024

Thank you for this PR, this is a great contribution to QUDA. I have done an initial visual review at this point, and left a bunch of comments. My only real concern of note is the demotion of the interface to being C89 instead of C99, but those concerns may be unwarranted.

Outside of those comments, somethings that will need to be done:

* [ ]  Apply clang-format (see [here](https://github.com/lattice/quda/wiki/Coding-Conventions-and-Style#coding-style-and-clang-format))

* [ ]  Create a wiki page of how to use OpenQ*D with QUDA, similar for example to what we have for [MILC}(https://github.com/lattice/quda/wiki/MILC-with-QUDA)

Is there any specific functionality that is added to QUDA that we should be adding to the internal tests?

Dear Kate,

I dealt with the two points:

  • see commit 568dd37 for the clang-formatting
  • The page for the wiki is written, but I cannot commit it, permission issue. I attach the patch here openqxd.zip

Best,
Roman

@maddyscientist
Copy link
Member

Hi Roman,

I've added you to the lattice github group, so you should have permission to edit the wiki now (and push to branches in the lattice org).

Thx

@maddyscientist
Copy link
Member

@Jenkins ok to test

@chaoos
Copy link
Author

chaoos commented Feb 28, 2024

I was able to commit to the wiki, thanks for giving me access!
The CI/CD also seems to be fixed.

@havogt
Copy link
Contributor

havogt commented Feb 28, 2024

cscs-ci run

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.

I've had a review of the latest code. This is a great contribution, but a few things left outstanding to be resolved. I have left some comments, but beyond those:

  • The C89 changes are of concern, since we will be embracing a C99 interface, e.g., use of bool.
  • The new openQCD interface is rather opaque to me. Can this be better documented?
  • Thanks for the wiki additions. Is there a test input file or equivalent to allow us to run a test with this?

@@ -2606,6 +2606,8 @@ namespace quda {
&& (pc_type == QUDA_MATPC_EVEN_EVEN || pc_type == QUDA_MATPC_EVEN_EVEN_ASYMMETRIC))
return true;

if (dirac_type == QUDA_WILSON_DIRAC || dirac_type == QUDA_CLOVER_DIRAC) return true;
Copy link
Member

Choose a reason for hiding this comment

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

This addition here looks bogus: the Wilson operator is not Hermitian. Is this addition intentional?

Copy link
Author

@chaoos chaoos Mar 26, 2024

Choose a reason for hiding this comment

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

This is in the class DiracG5M (i.e. gamma5 times the Dirac operator), which is Hermitian. I had to change this, since the eigensolver didn't accept a non-Hermitian operator else. So this should be correct (only for gamma5*D and D*gamma5 though).

@@ -38,6 +38,20 @@ namespace quda
#else
errorQuda("QUDA_RECONSTRUCT=%d does not enable reconstruct-8/9", QUDA_RECONSTRUCT);
#endif
#ifdef BUILD_OPENQCD_INTERFACE
Copy link
Member

Choose a reason for hiding this comment

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

@kostrzewa noting that we could augment this for tmLQCD after this is merged to give recon 9/13 support, which I guess will help your perf?

@chaoos
Copy link
Author

chaoos commented Mar 26, 2024

The C89 changes are of concern, since we will be embracing a C99 interface, e.g., use of bool.

Dear Kate,

  • The C99 thing is ok from our side. Should I just revert all changes I made in quda.h regarding this?
  • We are currently in the phase of documenting, so give me some days for this. Do you want more comments in the code (I think every function/method has a javadoc-decoration), or conceptual things in the wiki?
  • Regarding testing of openqxd functionality; we have some testing on the side of openqxd. If you want to test this in this repo, then you need to clone and compile the openqxd repo and run the tests from there. We are working on making this repo public, but there is still some politics going on: that's not in my power.

@maddyscientist
Copy link
Member

@chaoos apologies for the slow reply at my end. GTC and other travel got in the way.....

If you can revert the C99 -> C89 changes that would be great. It would also narrow the scope of the PR which would also be helpful.

Regarding documentation, the basic rule is that all new functions should have doxygen markup next to their declarations, together with overall conceptual documentation in the wiki. If you can write up the documentation to include cloning the openq*d and compiling the code to work with QUDA that would be great. Understood that the repo isn't public yet, and that's fine for now. Does that sound ok to you?

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.

8 participants