CEED / Laghos

High-order Lagrangian Hydrodynamics Miniapp
http://ceed.exascaleproject.org/miniapps
BSD 2-Clause "Simplified" License
189 stars 61 forks source link

Shifted energy factor calculation #161

Closed siuwuncheung closed 2 years ago

siuwuncheung commented 3 years ago

Shift energy factor calculation for time windowing when initial offset is used.

dylan-copeland commented 3 years ago

@siuwuncheung it makes sense to add the option to shift the singular values, and my only question is whether it is generally good to hard-code the shifting as done in this PR. What is the reason that shifting by 1 is always good when using initial offsets, for any problem? If there is a good reason, it should be documented. Or would it make sense to have an input parameter to set the shift?

siuwuncheung commented 3 years ago

@siuwuncheung it makes sense to add the option to shift the singular values, and my only question is whether it is generally good to hard-code the shifting as done in this PR. What is the reason that shifting by 1 is always good when using initial offsets, for any problem? If there is a good reason, it should be documented. Or would it make sense to have an input parameter to set the shift?

Theoretically shifting the singular value will give more accurate results but less speed-up with a larger dimension. But based on the experience, the numerical instability problem happens to insufficient basis dimension and the time step size eventually gets small (complete with no speed-up) or even vanishes (never complete). At the same energy factor level, the initial offset option gives less basis vectors than the interpolated offset, it does not actually achieve a better overall speed-up than the interpolated offset in most of the experiments (except the long-time Rayleigh-Taylor where interpolation does not work well). When I checked the basis number, sometimes the position basis just has 1 vector since itself occupies 99.99% energy factor and the original truncation is too aggressive), making the initial offset scheme useless. (We can always make energy factor larger to add more, but I believe it is better to have an “automatic” way to avoid the case of only 1 vector.) On the other hand, the offset in the interpolation option (or the deprecated previous state option) works like a dominant component, makes the decay of singular values more moderate. To make a fair comparison, I think we should not count the first singular value when the initial offset is used. I think it is fair to give up a bit of “theoretical speed-up” for a more reliable scheme using the initial offset, and perhaps we will see more scenarios where it is more useful than interpolation. If we want to leave the freedom of not shifting to compare the results, I can add an input parameter to set the shift.

dylan-copeland commented 3 years ago

@siuwuncheung thanks for the explanation, and I agree it is a good improvement. I think it would be better to make this a user input parameter, with a default value of 1, so that only an expert user would change it (maybe just to verify it is the best option). Also, some documentation about these issues would be helpful.

chldkdtn commented 3 years ago

@siuwuncheung thanks for the explanation, and I agree it is a good improvement. I think it would be better to make this a user input parameter, with a default value of 1, so that only an expert user would change it (maybe just to verify it is the best option). Also, some documentation about these issues would be helpful.

Making this a user input parameter with a default value of 1 sounds great to me.

chldkdtn commented 3 years ago

@siuwuncheung are all the things commented above addressed?

siuwuncheung commented 3 years ago

@siuwuncheung are all the things commented above addressed?

A user-defined option is added. I am setting the default value to be false (no shift), and performing the regression tests. All the regression tests are expected to pass. Once verified, we can turn the default value to be true.

siuwuncheung commented 3 years ago

Regression tests all passed in commit 08553333f8d349e669f9066e4bbb8528e0bbe83c. If everyone agrees, this branch can be merged after setting the default value of shift_sv to be true.