-
Notifications
You must be signed in to change notification settings - Fork 89
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
base: develop
Are you sure you want to change the base?
Conversation
Hi @rrsettgast Could you take a look at it and approve if it seems good to you? Thank you ! PS: I don't know why the docs build does not pass, it is only saying "We encountered a problem with a command while building your project. To resolve this error, double check your project configuration and installed dependencies are correct and have not changed recently." |
@sframba @rrsettgast @CusiniM can you review this please ? Thanks |
There was a problem hiding this 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.
if( m_timestepStabilityLimit==1 ) | ||
{ | ||
real64 dtOut = 0.0; | ||
computeTimeStep( dtOut ); | ||
m_timestepStabilityLimit = 0; | ||
m_timeStep=dtOut; | ||
} | ||
else | ||
{ | ||
EventManager const & event = getGroupByPath< EventManager >( "/Problem/Events" ); | ||
for( localIndex numSubEvent = 0; numSubEvent < event.numSubGroups(); ++numSubEvent ) | ||
{ | ||
EventBase const * subEvent = static_cast< EventBase const * >( event.getSubGroups()[numSubEvent] ); | ||
if( subEvent->getEventName() == "/Solvers/" + getName() ) | ||
{ | ||
m_timeStep = subEvent->getReference< real64 >( EventBase::viewKeyStruct::forceDtString() ); | ||
} | ||
} | ||
|
||
} | ||
|
||
if( m_useTaper==1 ) | ||
{ | ||
real32 vMin; | ||
vMin = getGlobalMinWavespeed( mesh, regionNames ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello,
The current definition is not optimal.
For example: When m_timestepStabilityLimit==1
a correct dt (m_timeStep/dtOut
) value is calculated, however, by setting m_timestepStabilityLimit==0
obliges all other calls to initializePostInitialConditionsPreSubGroups()
to use the user-input dt (either from xml or pygeosx). This scenario is often present when interfacing GEOS through python (pygeosx).
One option would be to never set m_timestepStabilityLimit==0
, however that implicates a dt calculation at each new call to initializePostInitialConditionsPreSubGroups()
. This may be useful for certain applications, but one often just wants to calculate the dt once. Besides that, at the current state, if initializePostInitialConditionsPreSubGroups()
is called a second time to calculate the dt (meaning that m_timestepStabilityLimit==1
is always true), the dt calculation does not work, giving nan
values.
After exploring a bit, the nan
values are due to a de-synchronization between GPU and CPU copies in the computeTimeStep
function. By calling initializePostInitialConditionsPreSubGroups()
, and therefore computeTimeStep
, a second time, the ux_n, uy_n, and uz_n arrays are not updated by the loop in lines 553-558, thus the division operations in lines 569-574 will give nan
.
A simple workaround is to avoid:
for( localIndex a = 0; a < sizeNode; ++a )
{
ux_n[a] = (real64)rand()/(real64) RAND_MAX;
uy_n[a] = (real64)rand()/(real64) RAND_MAX;
uz_n[a] = (real64)rand()/(real64) RAND_MAX;
}
simply writing:
for( localIndex a = 0; a < sizeNode; ++a )
{
ux_n[a] = 1.0;
uy_n[a] = 1.0;
uz_n[a] = 1.0;
}
and add the following loop to ensure the GPU values are not zero:
forAll< EXEC_POLICY >( sizeNode, [=] GEOS_HOST_DEVICE ( localIndex const a )
{
ux_n[a] = 1.0;
uy_n[a] = 1.0;
uz_n[a] = 1.0;
} );
Finally, it is not clear to me why we would need: return m_timeStep * m_cflFactor;
in line 678.
Notice that these comments are valid to the acoustic case also.
Coming back to the origin of the problem, it may be wise to add a third state m_timestepStabilityLimit==2
(for example). When m_timestepStabilityLimit==1
the dt is always calculated, if m_timestepStabilityLimit==2
the dt is calculated once and then set to state 3 (one that does not exist, implying that no further modification of m_timeStep
will take place), and if m_timestepStabilityLimit==0
the user-input dt is used (xml or pygeosx).
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; | ||
} |
There was a problem hiding this comment.
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:
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
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 [
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:
For this first version only second order solver contains the taper functionality and the computation of the profile as been simplified in particular in corner cases.
Those two functionalities will be add in a next PR.