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.

Sign up for GitHub

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

Jump to bottom

Squared singular values to calculate POD variance #306

Merged
merged 7 commits into from
Feb 5, 2025
Merged

Squared singular values to calculate POD variance #306

merged 7 commits into from
Feb 5, 2025

Conversation

axla-io
Copy link
Collaborator

@axla-io axla-io commented Feb 3, 2025

Current libROM implementation of BasisGenerator::finalSummary sums the singular values to calculate the total POD energy, but this should actually be the sum of squared singular values, compare to Chatterjee (2000) chapter 6.4. This short PR adresses that in libROM.

Test mixed nonlinear diffusion

To test with mixed nonlinear diffusion, run:

./mixed_nonlinear_diffusion -p 1 -offline -id 0 -sh 0.25
./mixed_nonlinear_diffusion -p 1 -offline -id 1 -sh 0.15
// Merge
./mixed_nonlinear_diffusion -p 1 -merge -ns 2

On the current master, the file mergedSV_FR reads

For energy fraction: 0.9, take first 5 of 200 basis vectors
For energy fraction: 0.99, take first 8 of 200 basis vectors
For energy fraction: 0.999, take first 12 of 200 basis vectors
For energy fraction: 0.9999, take first 16 of 200 basis vectors
For energy fraction: 0.99990, take first 16 of 200 basis vectors

In this PR, the file reads

For energy fraction: 0.9, take first 3 of 200 basis vectors
For energy fraction: 0.99, take first 5 of 200 basis vectors
For energy fraction: 0.999, take first 7 of 200 basis vectors
For energy fraction: 0.9999, take first 8 of 200 basis vectors
For energy fraction: 0.99990, take first 8 of 200 basis vectors

Test nonlinear elasticity

To test with nonlinear elasticity, run:

./nonlinear_elasticity_global_rom -offline -dt 0.01 -tf 5.0 -s 14 -vs 100 -sc 0.9 -id 0
./nonlinear_elasticity_global_rom -offline -dt 0.01 -tf 5.0 -s 14 -vs 100 -sc 1.1 -id 1

// Merge:
./nonlinear_elasticity_global_rom -merge -ns 2 -dt 0.01 -tf 5.0

On the current master, the file mergedSV_H.txt reads

For energy fraction: 0.9, take first 12 of 1002 basis vectors
For energy fraction: 0.99, take first 29 of 1002 basis vectors
For energy fraction: 0.999, take first 59 of 1002 basis vectors
For energy fraction: 0.9999, take first 96 of 1002 basis vectors
For energy fraction: 0.99999, take first 134 of 1002 basis vectors
For energy fraction: 0.999999, take first 165 of 1002 basis vectors
For energy fraction: 0.9999999, take first 189 of 1002 basis vectors
For energy fraction: 0.99999990, take first 189 of 1002 basis vectors

In this PR, the file reads

For energy fraction: 0.9, take first 5 of 1002 basis vectors
For energy fraction: 0.99, take first 11 of 1002 basis vectors
For energy fraction: 0.999, take first 19 of 1002 basis vectors
For energy fraction: 0.9999, take first 26 of 1002 basis vectors
For energy fraction: 0.99999, take first 35 of 1002 basis vectors
For energy fraction: 0.999999, take first 49 of 1002 basis vectors
For energy fraction: 0.9999999, take first 67 of 1002 basis vectors
For energy fraction: 0.99999990, take first 67 of 1002 basis vectors

@axla-io
Copy link
Collaborator Author

axla-io commented Feb 4, 2025

Following the approach in Dylan's corresponding Laghos PR, there is now a possibility to retrieve the old way of calculating energy fractions by calling:

./nonlinear_elasticity_global_rom -merge -ns 2 -dt 0.01 -tf 5.0 -no-sqsv
./mixed_nonlinear_diffusion -p 1 -merge -ns 2 -no-sqsv

@siuwuncheung
Copy link
Member

@axla-io Thanks for catching and fixing that! As the code style CI fails, you'll need to run astyle. Have you experienced with astyle before?

@mjkim1001 Seems the number of basis vectors is quite different. It might be better if we repeat the study again...

@axla-io
Copy link
Collaborator Author

axla-io commented Feb 4, 2025

@siuwuncheung Good catch, the latest commit is astyled.

@mjkim1001
Copy link
Collaborator

Looks good to me! Also, I can re-generate the results based on this PR.

@mjkim1001 mjkim1001 closed this Feb 4, 2025
@mjkim1001
Copy link
Collaborator

Reopening this PR since it was closed before merging.

@mjkim1001 mjkim1001 reopened this Feb 4, 2025
siuwuncheung
siuwuncheung reviewed Feb 5, 2025
View reviewed changes
examples/prom/mixed_nonlinear_diffusion.cpp Outdated Show resolved Hide resolved
examples/prom/nonlinear_elasticity_global_rom.cpp Outdated
@@ -328,7 +328,7 @@ void MergeBasis(const int dimFOM, const int nparam, const int max_num_snapshots,
generator.endSamples(); // save the merged basis file

int cutoff = 0;
generator.finalSummary(1e-7, cutoff, "mergedSV_" + name + ".txt");
generator.finalSummary(1e-7, cutoff, "mergedSV_" + name + ".txt",0,squareSV);
Copy link
Member

Choose a reason for hiding this comment

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

It is irrelevant to the functionality, but adding space after the commas make it consistent.

examples/prom/nonlinear_elasticity_global_rom.cpp Outdated
@@ -643,11 +646,11 @@ int main(int argc, char *argv[])
// Merge bases
if (x_base_only == false)
{
MergeBasis(true_size, nsets, max_num_snapshots, "V");
MergeBasis(true_size, nsets, max_num_snapshots, "V",squareSV);
Copy link
Member

Choose a reason for hiding this comment

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

Same here

examples/prom/nonlinear_elasticity_global_rom.cpp Outdated
MergeBasis(true_size, nsets, max_num_snapshots, "X");
MergeBasis(true_size, nsets, max_num_snapshots, "H");
MergeBasis(true_size, nsets, max_num_snapshots, "X",squareSV);
MergeBasis(true_size, nsets, max_num_snapshots, "H",squareSV);
Copy link
Member

Choose a reason for hiding this comment

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

Same here in these 2 lines

@axla-io
Copy link
Collaborator Author

axla-io commented Feb 5, 2025

Thanks @siuwuncheung this is resolved now. Interesting that astyle doesn't catch that

siuwuncheung
siuwuncheung reviewed Feb 5, 2025
View reviewed changes
examples/prom/mixed_nonlinear_diffusion.cpp Outdated
@@ -448,7 +448,7 @@ void MergeBasis(const int dimFOM, const int nparam, const int max_num_snapshots,
generator.endSamples(); // save the merged basis file

int cutoff = 0;
generator.finalSummary(1e-4, cutoff, "mergedSV_" + name);
generator.finalSummary(1e-4, cutoff, "mergedSV_" + name,0, squareSV);
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for asking this again.. there is one more comma in this line

examples/prom/nonlinear_elasticity_global_rom.cpp Outdated
@@ -328,7 +328,7 @@ void MergeBasis(const int dimFOM, const int nparam, const int max_num_snapshots,
generator.endSamples(); // save the merged basis file

int cutoff = 0;
generator.finalSummary(1e-7, cutoff, "mergedSV_" + name + ".txt");
generator.finalSummary(1e-7, cutoff, "mergedSV_" + name + ".txt",0, squareSV);
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for asking this again.. there is one more comma in this line.

Otherwise everything looks good to me!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@siuwuncheung Cool, in the latest commit, it's fixed.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @axla-io. Now this gets enough approvals. Please merge this PR to the master when you get a chance.

@axla-io axla-io merged commit d8cc32c into master Feb 5, 2025
4 checks passed
@axla-io axla-io deleted the merge-basis-squared-svals branch February 5, 2025 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@siuwuncheung siuwuncheung siuwuncheung approved these changes

@dylan-copeland dylan-copeland dylan-copeland approved these changes

@cval26 cval26 Awaiting requested review from cval26

@mjkim1001 mjkim1001 Awaiting requested review from mjkim1001

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@axla-io @siuwuncheung @mjkim1001 @dylan-copeland