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

ROM hyper reduction for density evaluation #268

Merged
merged 14 commits into from
Sep 16, 2024
Merged

ROM hyper reduction for density evaluation #268

merged 14 commits into from
Sep 16, 2024

Conversation

dreamer2368
Copy link
Collaborator

@dreamer2368 dreamer2368 commented Aug 9, 2024

testROMRhoOperator tests hyper-reduction for electron density evaluation.

  • computeRhoOnSamplePts evaluates electron density at the sampled grid points, given density matrix, orbital POD basis and ROM orbital coefficients.
    • This is equivalent to Rho::computeRho without rescaleTotalCharge.
  • Since rescaleTotalCharge requires every grid point information, it cannot be executed with hyper-reduction. It can be still performed after the electron density is projected onto ROM space via hyperreduction.
  • For code verification, exact equivalence between ROM and FOM can be achieved only when we have access to density before rescaling. This would require additional restart file save/load routines, which wouldn't be used at all in production runs. For now, the code is tested with adjusted error tolerance.

@dreamer2368
Copy link
Collaborator Author

@jeanlucf22 , this PR is still work-in-progress, but I'm wondering if I can get checked with you on one part. Can you take a look at line 517-550 of src/rom_workflows.cc, the function computeRhoOnSamplePts?

In essence, it computes the density rho only on sampled grid points, using libROM Matrix and Vector.

  • It is mainly based on computeRhoSubdomainUsingBlas3
  • For now we assume Mesh::subdivx() to be 1. This is the number of grids?
  • Also we assume density matrix is not distributed, so only 1 local matrix.

I'm wondering these assumptions are okay. These assumptions are also checked within the routines, so users won't misuse it unknowingly. Comparing with the slide, and also with computeRhoSubdomain, computeRhoSubdomainUsingBlas3 didn't seem to have a scalar factor of 2. The newly implemented function computeRhoOnSamplePts also does not include the factor of 2. I'm wondering if this is correct, or if I missed anything.

TODO(kevin): need to figure out what these functions do.
and probably need to make ROM-equivalent functions with another hyper-reduction?
*/
// gatherSpin();
Copy link
Collaborator

Choose a reason for hiding this comment

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

"spin" can be ignored for now. when spin is on, it is like having two replicated copy of everything, on two different sets of processors, and in this case rho is the sum of these two parts -> gather op

and probably need to make ROM-equivalent functions with another hyper-reduction?
*/
// gatherSpin();
// rescaleTotalCharge();
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is to make sure the charge is numerically exactly the physical charge. Sometimes, a slight rescaling is needed for that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We'll have to discuss this next week, but hyperreduction is not applicable for rescaleTotalCharge. We can still perform rescaling after (unscaled) density is projected onto ROM basis.
I mentioned a caveat for code verification above in the description.

@jeanlucf22
Copy link
Collaborator

@jeanlucf22 , this PR is still work-in-progress, but I'm wondering if I can get checked with you on one part. Can you take a look at line 517-550 of src/rom_workflows.cc, the function computeRhoOnSamplePts?

In essence, it computes the density rho only on sampled grid points, using libROM Matrix and Vector.

  • It is mainly based on computeRhoSubdomainUsingBlas3
  • For now we assume Mesh::subdivx() to be 1. This is the number of grids?
  • Also we assume density matrix is not distributed, so only 1 local matrix.

I'm wondering these assumptions are okay. These assumptions are also checked within the routines, so users won't misuse it unknowingly. Comparing with the slide, and also with computeRhoSubdomain, computeRhoSubdomainUsingBlas3 didn't seem to have a scalar factor of 2. The newly implemented function computeRhoOnSamplePts also does not include the factor of 2. I'm wondering if this is correct, or if I missed anything.

Indeed subdivx can be 1. It is an implementation optimization, sometimes using subdivx>1 to divide a subdomain associated with 1 MPI tasks into several sub-subdomains.
No factor 2 should be correct.

@dreamer2368
Copy link
Collaborator Author

Indeed subdivx can be 1. It is an implementation optimization, sometimes using subdivx>1 to divide a subdomain associated with 1 MPI tasks into several sub-subdomains. No factor 2 should be correct.

For now, computeRhoOnSamplePts keeps consistency with computeRhoSubdomainUsingBlas3 by not having the factor 2. This is the used routine for carbyne example. @jeanlucf22 , it'd be great if you can point me to where the factor 2 is multiplied outside of computeRhoSubdomainUsingBlas3, or whether computeRhoSubdomainUsingBlas3 needs to be fixed with factor 2.

@dreamer2368 dreamer2368 marked this pull request as ready for review September 6, 2024 19:10
@jeanlucf22
Copy link
Collaborator

Indeed subdivx can be 1. It is an implementation optimization, sometimes using subdivx>1 to divide a subdomain associated with 1 MPI tasks into several sub-subdomains. No factor 2 should be correct.

For now, computeRhoOnSamplePts keeps consistency with computeRhoSubdomainUsingBlas3 by not having the factor 2. This is the used routine for carbyne example. @jeanlucf22 , it'd be great if you can point me to where the factor 2 is multiplied outside of computeRhoSubdomainUsingBlas3, or whether computeRhoSubdomainUsingBlas3 needs to be fixed with factor 2.

The factor 2 should be in localX

@jeanlucf22
Copy link
Collaborator

@dreamer2368 Can you squash your commits so that at least the git commit comments are meaningful? Thanks

@dreamer2368
Copy link
Collaborator Author

@jeanlucf22 , if you're okay, I can do the squash and merge and edit the commit message there, since the squashing is the code practice here anyway.

@jeanlucf22
Copy link
Collaborator

@jeanlucf22 , if you're okay, I can do the squash and merge and edit the commit message there, since the squashing is the code practice here anyway.

Yes, please go ahead

@dreamer2368 dreamer2368 merged commit c40089b into ROMFPMD Sep 16, 2024
2 checks passed
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.

2 participants