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

feat: Add Taper Layers in wave solvers #3224

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

Conversation

acitrain
Copy link
Contributor

@acitrain acitrain commented Jul 12, 2024

This PR add a first version of Taper layers for wave solvers
These layers are set to improve the efficiency of absoption of boundaries.
This is how the version version works:

  • We compute a taper profile which is equal to one when you are outside the taper layers and to :
    $d_x = \mathrm{exp}\left(\displaystyle \frac{3V_{\mathrm{min}}}{2L}\mathrm{log}(R)\left(\frac{L-x}{L}\right)^2\cdot \Delta t\right)$ where $V_{\mathrm{min}}$ is the minimum P-wavespeed, L the length of the taper layer and R the reflectivitity coeff
  • Then we multiply by the array at time n and n+1 by the taperCoeff

For this first version only second order solver contains the taper functionality.

In a future PR, we can imagine to make the taper profile independant of the timestep and use the frequency instead.

acitrain added 30 commits May 2, 2024 16:53
@acitrain
Copy link
Contributor Author

@sframba @rrsettgast @CusiniM can you review this please ?

Thanks

@acitrain acitrain requested a review from rmadec-cs January 14, 2025 10:52
Copy link

@Victor-M-Gomes Victor-M-Gomes left a comment

Choose a reason for hiding this comment

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

This PR brings an important addition to the SEM wavepropagation module of GEOS. With this new absorbing boundary I've checked that one can achieve significantly better modelling results.

I suggest some small modifications to avoid some problems encountered during testing.

Comment on lines +118 to +125
if( dist<sizeT )
{
taperCoeff[a] = LvArray::math::exp((((3*vMin)/(2*sizeT))*log( r )*pow((sizeT-dist)/sizeT, 2 ))*dt );
}
else
{
taperCoeff[a] = 1.0;
}

Choose a reason for hiding this comment

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

Great job with the taper, choosing vMin over vMax was a really good choice since it usually leads to better results. This needs to be modified in the description of the PR though:
$d_x = \displaystyle \frac{-3V_{max}}{2L}log(R)(\frac{x}{L})^2$ becomes $d_x = \exp\left( \displaystyle \frac{3V_{min}}{2L}log(R)(\frac{L-x}{L})^2 dt \right)$ (the dt also appears in the definition).

After much testing the border reflections have been successfully attenuated and modelling results are significantly improved.

For the future (in another PR maybe) I would suggest substituting $V_{min} \times dt$ by some $\lambda_{min} = V_{min}/f_{max}$, where $f_{max}$ is the maximum frequency that one wishes to consider in the modelling. Right now $f_{max} == f_{sampling}$, the maximum frequency that exists in the data. By allowing the user to input $\lambda_{min}$ or $f_{max}$, the equation would have more physical meaning and the use of $dt$ could be avoided. Another option would be to set $f_{max} \simeq f_{Ricker} \times 3$ (or a similar factor) to avoid defining new inputs. The goal of the suggestion is to avoid using dt in the definition of the taper, while also inserting additional physical meaning.

Finally, it may be important to mention that the choice of L and R are somewhat empirical. For a given R, the greater the L, the better the attenuation, but that also comes with the cost of increasing the model to fit the taper. Thus, in general a compromise between L and R need to be found. For the bounds of R, I would (based on some tests) place them in the interval [ $10^{-3}$ , 1).

Copy link
Contributor

Choose a reason for hiding this comment

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

@Victor-M-Gomes I have changed the PR description accordingly

Copy link
Contributor

@sframba sframba Jan 29, 2025

Choose a reason for hiding this comment

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

Re the dt, I don't find it so absurd having a dt in the formula, since if you cut the timestep in two, you want to apply the same factor as you did before for a given time. But I guess the other parameters are also heurisitc... Thoughts?

@acitrain
Copy link
Contributor Author

@rrsettgast @CusiniM can one of you take a look to this?
It is ok on our side we just need a code owner review to go to the merge queue and do integrated test when this PR will be at the top

Thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci: run code coverage enables running of the code coverage CI jobs ci: run CUDA builds Allows to triggers (costly) CUDA jobs ci: run integrated tests Allows to run the integrated tests in GEOS CI flag: ready for review flag: requires rebaseline Requires rebaseline branch in integratedTests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants