Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Issue 0133 create new spyro version were every dependency not pip installable is optional #137
Issue 0133 create new spyro version were every dependency not pip installable is optional #137
Changes from 8 commits
2240097
f011fc7
5fb8c38
0a9e8dd
0c67ed5
457c91c
749ebba
3f1f690
498354a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
Check warning on line 7 in spyro/meshing/meshing_functions.py
Codecov / codecov/patch
spyro/meshing/meshing_functions.py#L6-L7
Check warning on line 229 in spyro/meshing/meshing_functions.py
Codecov / codecov/patch
spyro/meshing/meshing_functions.py#L229
Check warning on line 268 in spyro/meshing/meshing_functions.py
Codecov / codecov/patch
spyro/meshing/meshing_functions.py#L268
Check warning on line 26 in spyro/solvers/acoustic_wave.py
Codecov / codecov/patch
spyro/solvers/acoustic_wave.py#L25-L26
Check warning on line 118 in spyro/solvers/acoustic_wave.py
Codecov / codecov/patch
spyro/solvers/acoustic_wave.py#L117-L118
Check warning on line 74 in spyro/tools/velocity_smoother.py
Codecov / codecov/patch
spyro/tools/velocity_smoother.py#L74
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 test will run MLT or spectral methods depending on the environment. In either case, some part of the code will not be tested. I think the test environment should have SeismicMesh installed and run the test for every method that is supported by spyro.
We may also have another environment or mock approach for testing the logic of SeismicMesh availability. However, I guess this is not the scope of this test in particular.
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.
You are absolutely right about the importance of running the test for both types of 2D elements, especially to ensure comprehensive coverage. The reason I initially tested only one of these was that this particular test takes a significant amount of time to complete in our runner. However, I agree that we should incorporate both and fixed this in the latest commit.
As for testing the logic of
SeismicMesh
availability, I did attempt to implement something using theunittest
package and mocks, but I encountered some unexpected issues that I couldn't fully resolve at the time. I plan to revisit this. If you have any suggestions or best practices for handling this kind of dependency testing (e.g., using mocks or creating a lightweight testing environment), I would greatly appreciate your inputThere 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.
In this case we can try reducing the memory requirements of the test instead of skipping it. For example, we can try increasing "gradient_sampling_frequency". I left a low (and probably inappropriate) value for this field.
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 is a good idea. Once the demeter runner finishes the current jobs, I will SSH into it and try out some different sampling values to see if I can reduce memory requirements without compromising the test.
I am working from home because of the holiday. Therefore, I don't want to risk loosing SSH access to my personal workstation in the lab (that only has 16 GB RAM) by accident while testing this. Demeter has significant more RAM (48 GB) and can run this test as is without a problem.