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

FIX: Adding allocation for evals in MG when deflation is preserved #1377

Merged
merged 9 commits into from
Jan 17, 2024

Conversation

pittlerf
Copy link
Contributor

@pittlerf pittlerf commented May 15, 2023

In the current develop version I obtained the following error when updating MG with preserve_deflation.

MG level 1 (GPU): ERROR: Requesting 256 eigenvalues with only storage allocated for 0 (rank 0, host nid007567, eigensolve_quda.cpp:582 in void quda::EigenSolver::computeEvals(std::vector<ColorSpinorField> &, std::vector<Complex> &, int)())

Here I provide a small fix. Thank you,

@pittlerf pittlerf requested a review from a team as a code owner May 15, 2023 07:19
@mathiaswagner
Copy link
Member

Jenkins: Can one of the admins verify this patch?

1 similar comment
@mathiaswagner
Copy link
Member

Jenkins: Can one of the admins verify this patch?

@maddyscientist
Copy link
Member

@Jenkins ok to test

@maddyscientist
Copy link
Member

@pittlerf thanks for the contribution. Please feel free add your name to the contributor list on the README 😄

@maddyscientist
Copy link
Member

@Jenkins ok to test

@kostrzewa
Copy link
Member

This does not seem to be enough to make it work for me. I need to additionally resize evals before or when EigenSolver::computeEvals is called.

@kostrzewa
Copy link
Member

As a temporary workaround I do the following:

584   void EigenSolver::computeEvals(std::vector<ColorSpinorField> &evecs,
585                                  std::vector<Complex> &evals, int size)
586   {
587     if (size > (int)evecs.size())
588       errorQuda("Requesting %d eigenvectors with only storage allocated for %lu", size, evecs.size());
589     if (size > (int)evals.size())
590       evals.resize(size);                                                                                                              
591       //errorQuda("Requesting %d eigenvalues with only storage allocated for %lu", size, evals.size());

@weinbe2
Copy link
Contributor

weinbe2 commented Aug 8, 2023

@kostrzewa @pittlerf trying to close the loop on this---I know the PR's been stagnant a bit, can one of you please merge in develop (which should "just work", it seems), add that change in computeEvals, and do a last test with your workflow (since I don't know what exactly it is)?

I can take care of cosmetic things like updating contributors + clang-format if that helps, I just can't confirm that the fixes here still work for you.

@kostrzewa
Copy link
Member

@weinbe2 sorry, I completely lost sight of this for a number of reasons, have it on my TODO list now and will get back to you

@pittlerf
Copy link
Contributor Author

@kostrzewa @pittlerf trying to close the loop on this---I know the PR's been stagnant a bit, can one of you please merge in develop (which should "just work", it seems), add that change in computeEvals, and do a last test with your workflow (since I don't know what exactly it is)?

I can take care of cosmetic things like updating contributors + clang-format if that helps, I just can't confirm that the fixes here still work for you.

Hi, yes, sorry I also lost sight of this, however with @kostrzewa additions still works for our framework.

@kostrzewa
Copy link
Member

@weinbe2 I can confirm that this still works correctly with tmLQCD after the merge with develop. I've removed the commented-out error message and added a comment to explain why the evals.resize() is necessary. Not sure if the latter is really helpful.

Should I squash these into a single commit and force-push?

@maddyscientist
Copy link
Member

No need to force-push I think, we can I assume just do a "Squash and Merge"?

I've merged in latest develop, which should allow the CI matrix to complete. Once that shows as green, we can just hit the button I think.

@maddyscientist
Copy link
Member

@Jenkins test this please

@maddyscientist maddyscientist merged commit ef9c9b2 into develop Jan 17, 2024
12 checks passed
@maddyscientist maddyscientist deleted the hotfix/preserve_deflation branch January 17, 2024 18:57
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.

5 participants