-
Notifications
You must be signed in to change notification settings - Fork 224
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
Figure.meca: Refactor the tests for pass a single focal mechanism to spec #2219
Conversation
Summary of changed imagesThis is an auto-generated report of images that have changed on the DVC remote
Image diff(s)Report last updated at commit 61c4f2a |
@@ -54,7 +118,7 @@ def test_meca_spec_dict_list(): | |||
|
|||
|
|||
@pytest.mark.mpl_image_compare | |||
def test_meca_spec_dataframe(): | |||
def test_meca_spec_two_dataframe(): |
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.
Why is this being renamed? As far as I can tell there is just a single DataFrame in this function.
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.
I was planning to have two groups of tests:
- In tests
test_meca_spec_*
, a single focal mechanism in different data types (dict, DataFrame, 1darray and plaintext file) is passed to thespec
parameter - In tests
test_meca_spec_two_*
, two focal mechanisms in different data types (dict with list as keys, DataFrame, 2D list, 2D array and plaintext file) are passed to thespec
parameter.
That's why I added the _two
suffix to the tests, but I agree that's not a good name for these tests. Do you have better ideas?
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.
That makes sense. I think it will be hard to come up with a descriptive test name without making it too wordy. I think it might just be easier to update the docstring (I made my suggestion below) to make it clear that it's passing two focal mechanisms.
Test supplying a pandas.DataFrame containing focal mechanisms and locations | ||
to the spec parameter. |
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.
Test supplying a pandas.DataFrame containing focal mechanisms and locations | |
to the spec parameter. | |
Test supplying two focal mechanisms and their locations by passing a pandas.DataFrame | |
to the spec parameter. |
I think this makes it more clear why "two" is in the function name.
I'm closing the issue and will open another PR to reorganize the meca tests later. |
Description of proposed changes
The
Figure.meca
tests are a little messy and difficult to maintain because each test uses different focal mechanisms and event information. I plan to refactor the tests and this PR is the first step.This PR focuses on testing passing a single focal mechanism to the
spec
parameter using different data types (dict, file, 1darray and DataFrame). Note that all tests now single a single baseline imagetest_meca_spec_dict.png
.Changes in this PR: