-
Notifications
You must be signed in to change notification settings - Fork 69
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
Add option to set EQLHarmonic points to constant depth #236
Conversation
Rename the relative_depth parameter for depth and add a new depth_type parameter. Keep supporting the relative_depth parameter but raise a FutureWarning if passed. Add a new build_points method for creating the point source coordinates based on the data points.
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.
Just a small suggestion to parametrize the test functions. Otherwise looking really good!
harmonica/tests/test_eql_harmonic.py
Outdated
points = eql._build_points(coordinates) | ||
# Check output | ||
expected_points = (easting, northing, upward - 4.5e3) | ||
npt.assert_allclose(points, expected_points) |
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.
Best to split these tests into multiple ones. Use a fixture for the data and parametrize the options. Otherwise it's hard to tell which part breaks when tests fail.
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.
Sure!
Co-authored-by: Leonardo Uieda <[email protected]>
Co-authored-by: Leonardo Uieda <[email protected]>
Define a coordinates fixture and parametrize the depth_type and expected upward array. Create a separate test function for testing the relative_depth parameter. By making it independent from the other test function, we are making it easier for deprecating it in the future.
I've improved the tests by defining a BTW, I used the
Eg: @pytest.fixture(name="coordinates")
def fixture_coordinates():
... Source: https://stackoverflow.com/questions/46089480/pytest-fixtures-redefining-name-from-outer-scope-pylint |
Rename the
relative_depth
parameter fordepth
and add a newdepth_type
parameter. Keep supporting the
relative_depth
parameter but raisea
FutureWarning
if passed. Add a new_build_points()
method for creatingthe point source coordinates based on the data points. Add test functions for
the new feature using
pytest.fixture
for sample coordinates. Make examples touse the new
depth
argument.Reminders:
make format
andmake check
to make sure the code follows the style guide.doc/api/index.rst
and the base__init__.py
file for the package.AUTHORS.md
file (if you haven't already) in case you'd like to be listed as an author on the Zenodo archive of the next release.