Issue 0041 - Add elastic wave propagation #53
Conversation
spyro/io/model_parameters.py
Outdated
There was a problem hiding this comment.
I removed some extra empty spaces from some warning messages. This is not related to the actual development of elastic waves.
There was a problem hiding this comment.
I renamed the _get_initial_velocity_model to _initialize_model_parameters because the elastic wave propagators have more parameters. I also moved the implementation to AcousticWave to allow ElasticWave to inherit Wave behavior consistently.
| ) | ||
| self.initial_velocity_model_file = vp_filename + ".hdf5" | ||
|
|
||
| if self.initial_velocity_model_file.endswith((".hdf5", ".h5")): |
There was a problem hiding this comment.
I added .h5 as an alternative to .hdf5 because it is another common extension for HDF5.
…ed_space_central_difference' function into a unique function
spyro/utils/typing.py
Outdated
There was a problem hiding this comment.
This can be done as soon as Firedrake fixes the GLL quads numbering issue, since it is compatible with 3.12. If they take too long I can look into it after turning in my thesis.
There was a problem hiding this comment.
This is not a really important feature (we don't need to set it as a priority), but I think it helps following up which methods are overriding something from the parent class.
|
|
||
| @abstractmethod | ||
| def _initialize_model_parameters(self): | ||
| pass |
There was a problem hiding this comment.
Good idea moving this to the other classes
There was a problem hiding this comment.
I think it will help because other models have more parameters (e.g., density and S-wave velocity).
|
|
||
| @abstractmethod | ||
| def _get_next_vstate(self): | ||
| pass |
There was a problem hiding this comment.
I really like what you have done here. Your changes significantly improve the readability and maintainability of the subsequent time integration methods. However, to ensure there are no unintended increases in memory usage or runtime, I recommend we generate flame graphs for verification.
There was a problem hiding this comment.
I believe it would not increase the runtime considerably because the numerical parts of the code are probably the bottlenecks. I will work on the flame graphs to make sure that these modifications did not degrade the code performance.
There was a problem hiding this comment.
It's been a while since I last did a flame graph, but I found this resource from the Firedrake project to be quite useful for code optimization: https://www.firedrakeproject.org/optimising.html. You don't have to do it, you can just time the 3d test and the time_convergence test locally to see if nothing major changed. When we go to elastic 3D, we should generate flamegraphs to try and find any unnecessary bottlenecks
…tral_difference_MMS' function into a unique function
|
|
||
| @override | ||
| def update_source_expression(self, t): | ||
| self.q_t.assign(2*t) |
There was a problem hiding this comment.
I'm uncertain if this is the best practice. While using MMS for this small problem might not cause any issues, for larger cases in the future, we should prioritize wrapping scalar time-dependent variables inside a fire.Constant for better performance.
There was a problem hiding this comment.
I didn't get the issue here. The variable "q_t" is defined as a Constant is line 14. This should be faster than before because the method was returning new constants in every call. As far as I remember, using the same constant avoids some parts of JIT or assembling.
There was a problem hiding this comment.
Ah, I see now. I mistakenly thought that q_t was defined as a Function rather than a Constant, I didn't see line 14. Thanks for clarifying. Yes, reusing the same constant does indeed optimize the process by avoiding the overhead associated with redefining it repeatedly, leading to better performance.
I'm creating a draft pull request for discussing the implementations/modifications and avoiding a big review. Initially I just added the directory structure and the input parameters for the elastic model.
I think we need a ElasticWave base class with common code for IsotropicElasticWave, TTLElasticWave, ViscoElasticWave, etc. The parsing of the parameters dictionary is used as an example to illustrate why I think the base class would help us adding new features.