Create tests for utils/plotting.py (#302) #323
Conversation
|
@klieret For now, I have only created tests for |
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #323 +/- ##
==========================================
+ Coverage 69.67% 72.84% +3.17%
==========================================
Files 44 45 +1
Lines 2463 2504 +41
==========================================
+ Hits 1716 1824 +108
+ Misses 747 680 -67
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks a lot for all of your work. I'm impressed by how thoroughly you started this. I was mostly thinking about ensuring that the plots run without failure, rather than testing the results.
This is also where I see the following issue: We duplicate a lot of code from the EventPlotter. That means we test that we get the same result with the code copied over from its origin (with the intermediate step of plotting it and retrieving it again), which then doesn't show all that much.
So while I can see that a lot of thinking went into this, would you be open to simplifying this?
So we could do something really silly, like just testing len(fig.axes[i].get_lines()[0].get_xydata())).
If we want to test the content, we could simply do something like
fig.axes[i].get_lines()[0].get_xydata()[0] == approx(hard coded number here)
| from gnn_tracking.utils.plotting import EventPlotter | ||
|
|
||
|
|
||
| @patch("matplotlib.pyplot.show") |
| fig, _ = plotter.plot_ep_rv_uv(evtid=evtid) | ||
| plotted_data_points = [] | ||
| original_data_points = [] | ||
| original_data_points.append(hits[["eta", "phi"]].values) |
There was a problem hiding this comment.
it's recommended to use DataFrame.to_numpy() rather than .values
| for i in range(3): | ||
| plotted_data_points.append(np.array(fig.axes[i].get_lines()[0].get_xydata())) | ||
| plt.close(fig) | ||
| assert (plotted_data_points[0] == original_data_points[0]).all() |
There was a problem hiding this comment.
since these are floating points, it's probably best to use pytest.approx or something similar
| original_data_points.append(hits[["z", "r"]].values) | ||
| original_data_points.append(hits[["u", "v"]].values) | ||
| for i in range(3): | ||
| plotted_data_points.append(np.array(fig.axes[i].get_lines()[0].get_xydata())) |
There was a problem hiding this comment.
Nice! That's a cool way of actually testing the real content
|
So, am I wrong in assuming that you need the test to look something like this? def test_event_plotter(mock_show):
assert mock_show is plt.show
path = trackml_test_data_dir
evtid = 1
plotter = EventPlotter(indir=path)
fig, _ = plotter.plot_ep_rv_uv(evtid=evtid)
original_data_points = []
original_data_points.append(
[
[-3.65964174, -2.99420524],
[-3.14542103, -2.94588566],
[-3.65947032, -2.99451041],
[-3.14519644, -2.94551826],
[-2.90377235, -2.63840151],
[-3.21849656, -2.51005006],
[-3.2187562, -2.50965071],
[-3.48590827, -2.21800613],
[-2.85355783, -2.35087061],
[-3.21562123, -2.23991871],
]
)
original_data_points.append(
[
[-1502.5, 77.40522003],
[-1498.0, 129.21363831],
[-1498.0, 77.18669128],
[-1502.0, 129.58804321],
[-1498.0, 164.72381592],
[-1502.0, 120.39829254],
[-1498.0, 120.04631805],
[-1498.0, 91.84147644],
[-1502.0, 173.72445679],
[-1498.0, 120.42472839],
]
)
original_data_points.append(
[
[-0.01277896, -0.00189722],
[-0.00759138, -0.00150495],
[-0.01281572, -0.00189868],
[-0.0075689, -0.00150338],
[-0.00531829, -0.00292747],
[-0.00670373, -0.00490364],
[-0.00672142, -0.00492071],
[-0.00656526, -0.00868637],
[-0.00404855, -0.00409189],
[-0.00515092, -0.00651333],
]
)
for i in range(3):
assert_approx_equal(
np.array(fig.axes[i].get_lines()[0].get_xydata()[:10]),
np.array(original_data_points[i]),
)
plt.close(fig) |
|
Yes, I think that looks perfect :) Some code quality checkers will probably want you to define I propose to merge this once you're done with this test and to create a new PR for any other tests. |
|
I have pushed the changes. Let me know if any changes are required. |
There was a problem hiding this comment.
Looks good to me! Thank you so much :)
I'll merge once the tests run through
for i in range(3):
> assert_approx_equal(
np.array(fig.axes[i].get_lines()[0].get_xydata()[:10]),
np.array(original_data_points[i]),
)
src/gnn_tracking/utils/test_plotting.py:60:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
args = (array([[-3.65964174, -2.99420524],
[-3.14542103, -2.94588566],
[-3.65947032, -2.99451065],
[-3.1...2.50965071],
[-3.48590827, -2.21800613],
[-2.85355783, -2.35087061],
[-3.21562123, -2.23991871]]))
kwds = {}
@wraps(func)
def inner(*args, **kwds):
with self._recreate_cm():
> return func(*args, **kwds)
E TypeError: only size-1 arrays can be converted to Python scalars
I don't immediately see the route cause, does this happen locally?
|
oops test failed because I forgot to change |
|
@all-contributors please add @kingjuno for test |
|
We had trouble processing your request. Please try again later. |
|
@all-contributors please add @kingjuno for test |
|
We had trouble processing your request. Please try again later. |
|
Hmmm :/ I wonder if I broke the Added manually |
This PR creates tests for #302.