Extending test coverage for 2D visualization by TRY-ER · Pull Request #5000 · MDAnalysis/mdanalysis

TRY-ER · 2025-03-30T11:39:51Z

Issue 597

GSoC Primary Contribution for test coverage!

Changes made in this Pull Request:

Previously, the visualization.streamlines.py showed 54% of the test coverage.
The limitations were mostly due to the declaration of certain functions within another function
The functions were separated and corresponding test cases were added.
The case of maximum core utilized were handled.

Increase by 38 % code coverage in the specific file

PR Checklist

Issue raised/referenced?
Tests updated/added?
Documentation updated/added?
package/CHANGELOG file updated?
Is your name in package/AUTHORS? (If it is not, add it!)

Developers Certificate of Origin

I certify that I can submit this code contribution as described in the Developer Certificate of Origin, under the MDAnalysis LICENSE.

📚 Documentation preview 📚: https://mdanalysis--5000.org.readthedocs.build/en/5000/

github-actions

Hello there first time contributor! Welcome to the MDAnalysis community! We ask that all contributors abide by our Code of Conduct and that first time contributors introduce themselves on GitHub Discussions so we can get to know you. You can learn more about participating here. Please also add yourself to package/AUTHORS as part of this PR.

codecov · 2025-03-30T11:55:43Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.58%. Comparing base (f960aa1) to head (afdd4f8).
Report is 18 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #5000      +/-   ##
===========================================
+ Coverage    93.43%   93.58%   +0.15%     
===========================================
  Files          177      189      +12     
  Lines        21894    22960    +1066     
  Branches      3095     3095              
===========================================
+ Hits         20457    21488    +1031     
- Misses         986     1019      +33     
- Partials       451      453       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

orbeckst

Quick surface-level review: Given that these functions were previously private, we should keep them out of the public API by naming them with a leading underscore:

_produce_list_indices_point_in_polygon_this_frame()
_produce_list_centroids_this_frame()
Then they also don't have to show up in docs.

Also add yourselves to AUTHORS.

We don't need a CHANGELOG entry for tests/internal refactoring.

orbeckst · 2025-03-30T16:36:26Z

@lilyminium @tylerjereddy would you be able to add a review — should be quick. Please leave an accept or changes requested to be clear about your intentions. This is GSOC relevant so needs to get done soon and also needs to be unambiguous. Thank you!

tylerjereddy · 2025-03-31T02:29:10Z

done soon

By end of day Monday or sooner?

orbeckst · 2025-03-31T02:56:23Z

End of Monday works!

…coverage

TRY-ER · 2025-03-31T05:10:43Z

Quick surface-level review: Given that these functions were previously private, we should keep them out of the public API by naming them with a leading underscore:

_produce_list_indices_point_in_polygon_this_frame()

_produce_list_centroids_this_frame()
Then they also don't have to show up in docs.

Also add yourselves to AUTHORS.

We don't need a CHANGELOG entry for tests/internal refactoring.

Made the notified changes !

lilyminium

Great additions to the test coverage @TRY-ER, I've added some suggestions below.

testsuite/MDAnalysisTests/visualization/test_streamlines.py

lilyminium · 2025-03-31T07:48:21Z

testsuite/MDAnalysisTests/visualization/test_streamlines.py

+    indices_tuple = (np.array([1, 3]),)
+    list_indices = [indices_tuple]
+    result = streamlines._produce_list_centroids_this_frame(list_indices, pts)
+    expected = np.average(pts[[1, 3]], axis=0)


Suggested change

expected = np.average(pts[[1, 3]], axis=0)

expected = np.array([2., 2.])

It's better to test actual values instead of re-running through the function logic.

Added actual values directly!

lilyminium · 2025-03-31T07:48:56Z

testsuite/MDAnalysisTests/visualization/test_streamlines.py

+    expected1 = np.average(pts[[0, 2]], axis=0)
+    expected2 = np.average(pts[[1, 3, 4]], axis=0)


Here as well could you please use the actual values?

Added actual values !

lilyminium · 2025-03-31T07:52:48Z

testsuite/MDAnalysisTests/visualization/test_streamlines.py

+        (np.array([0, 0]), [0.7999992370605469, 0.5399990081787109]),
+        (np.array([1, 0]), [0.8000001907348633, 0.5399971008300781]),
+        (np.array([2, 0]), [0.8000020980834961, 0.5400047302246094]),
+        (np.array([3, 0]), [0.8000001907348633, 0.5400009155273438]),
+        (np.array([4, 0]), [0.7999982833862305, 0.5400009155273438]),
+        (np.array([0, 1]), [0.7999992370605469, 0.5399999618530273]),
+        (np.array([1, 1]), [0.7999954223632812, 0.5400009155273438]),
+        (np.array([2, 1]), [0.7999992370605469, 0.5400047302246094]),
+        (np.array([3, 1]), [0.7999954223632812, 0.5399932861328125]),
+        (np.array([4, 1]), [0.8000030517578125, 0.5399932861328125]),
+        (np.array([0, 2]), [0.8000068664550781, 0.5399999618530273]),
+        (np.array([1, 2]), [0.7999992370605469, 0.5400009155273438]),
+        (np.array([2, 2]), [0.8000106811523438, 0.5400009155273438]),
+        (np.array([3, 2]), [0.8000106811523438, 0.5399932861328125]),
+        (np.array([4, 2]), [0.8000030517578125, 0.5399932861328125]),
+        (np.array([0, 3]), [0.7999954223632812, 0.5399999618530273]),
+        (np.array([1, 3]), [0.7999954223632812, 0.5400009155273438]),
+        (np.array([2, 3]), [0.8000030517578125, 0.5399971008300781]),
+        (np.array([3, 3]), [0.8000030517578125, 0.5399932861328125]),
+        (np.array([4, 3]), [0.8000030517578125, 0.5400009155273438]),
+        (np.array([0, 4]), [0.79998779296875, 0.5400009155273438]),
+        (np.array([1, 4]), [0.8000106811523438, 0.5399971008300781]),
+        (np.array([2, 4]), [0.7999954223632812, 0.5400047302246094]),
+        (np.array([3, 4]), [0.8000030517578125, 0.5400009155273438]),
+        (np.array([4, 4]), [0.7999954223632812, 0.5399932861328125]),


It looks like these are all roughly (0.8, 0.54) -- could you use the atol parameter to check for that instead of including such fine precision?

As suggested, a check logic was assigned to check the approximate values to 0.8 and 0.5. (rather checking whole output)

tylerjereddy

At first glance, I think I agree with Lily--this cleans up some nested functions I never should have written that way (the code is very old, I was still learning) and exposes them to some tests.

The tests may be mostly around basic functionality, but that still has some value of course. I added a few inline comments/suggestions.

One more thing--the description of this PR cites gh-597 and I don't see the connection--that issue is about checking that errors are raised properly in our source code by testing faulty inputs, etc. The only relationship to this PR is the theme of improving test coverage, but that particular issue is about that specific type of test coverage, for ensuring that errors are raised as expected.

tylerjereddy · 2025-03-31T18:55:54Z

testsuite/MDAnalysisTests/visualization/test_streamlines.py

+    list_indices = [indices_tuple]
+    result = streamlines._produce_list_centroids_this_frame(list_indices, pts)
+    expected = np.array([2.0, 2.0])
+    np.testing.assert_array_almost_equal(result[0], expected)


We should prefer assert_allclose in new code per the docstring of this NumPy function I think.

Used assert_allclose in recent changes.

tylerjereddy · 2025-03-31T18:57:28Z

testsuite/MDAnalysisTests/visualization/test_streamlines.py

+            [0.5, 0.5],
+            [1.5, 0.5],
+        ]
+    )


minor: I think we could write this more compactly

Not sure why, but the black linting was giving errors, hence wrote the list in this manner.

Even though I agree with @tylerjereddy , I am going to succumb to black's opinion here and just leave it in whichever way black wants it to be. Not worthwhile to mark it as an exception.

tylerjereddy · 2025-03-31T19:02:05Z

testsuite/MDAnalysisTests/visualization/test_streamlines.py

+        vertex_list, points
+    )
+    expected = [(np.array([0], dtype=int),)]
+    np.testing.assert_array_equal(result[0][0], expected[0][0])


Maybe I'm misunderstanding something, but above it says:

matplotlib.path.Path.contains_points does not include boundary points.

But expected has the index-0 point detected as interior, no?

My bad, I just checked the boundary points that are included in the Matplotlib path and returns a 1D zero Numpy array. Made changes to the comment.

I think you're just checking the 0-index here, which wouldn't check if result had more than one entry -- could you convert both result and expected to arrays?

In this specific test, I was checking a single point on the shape's boundary! As there will be one expected element in the array, I tested only the first element in the initial test. As you commented, I have added to check all the items in the list rather than the first one only.

tylerjereddy · 2025-03-31T19:05:13Z

testsuite/MDAnalysisTests/visualization/test_streamlines.py

+    for entry in values:
+        res = entry[1]
+        assert res[0] == pytest.approx(0.8, abs=1e-1)
+        assert res[1] == pytest.approx(0.5, abs=1e-1)


probably minor, but I suspect using assert_allclose here with a slice of the first two indices will tell us if only one of these is failing while as written the first failure could mask the success/failure of the second assertion

Done it with the numpy.assert_allclose() !

tylerjereddy · 2025-03-31T19:11:26Z

testsuite/MDAnalysisTests/visualization/test_streamlines.py

+        ymin=univ.atoms.positions[..., 1].min(),
+        ymax=univ.atoms.positions[..., 1].max(),
+        maximum_delta_magnitude=2.0,
+        num_cores="maximum",


I might be a little hesitant to test num_cores="maximum" by default, at least in the CI. One option might be to have a slow test marker for the multi-core test and only do that locally/when needed, but I'm not sure if we have such a marker at the moment (it is quite common upstream for resource-intensive tests where you occasionally may want to check/bisect, but don't want to/can't run continuously).

I suspect the pragmatic thing for now may be to just use a single core, or perhaps 2 so that we get concurrency with reduced risk of locking things up.

The test case was added to consider the core inclusion case of "maximum". I have modified the cores to 2. If there is such a marker implemented later, I will modify the test case accordingly to test if the "maximum" case works as expected.

TRY-ER · 2025-04-01T05:59:48Z

At first glance, I think I agree with Lily--this cleans up some nested functions I never should have written that way (the code is very old, I was still learning) and exposes them to some tests.

The tests may be mostly around basic functionality, but that still has some value of course. I added a few inline comments/suggestions.

One more thing--the description of this PR cites gh-597 and I don't see the connection--that issue is about checking that errors are raised properly in our source code by testing faulty inputs, etc. The only relationship to this PR is the theme of improving test coverage, but that particular issue is about that specific type of test coverage, for ensuring that errors are raised as expected.

You are right that the notified issue does not entirely align with the contribution. But among the issues tagged as GSoC Starter, the contribution was most relevant for test case coverage.

orbeckst

Overall looks good to me. As formalities

add your GH handle to the author list for the 2.10 release in CHANGELOG – we don't need an entry but you should be visible as a contributor
fix author order in AUTHORS

@lilyminium and @tylerjereddy please let me know if you have any remaining concerns/comments. Thank you for reviewing, much appreciated!!

orbeckst · 2025-04-03T00:45:34Z

package/AUTHORS

  - Matthew Davies
  - Jia-Xin Zhu
  - Tanish Yelgoe
- 2025


oops, thanks for fixing :-)

orbeckst · 2025-04-03T00:46:02Z

package/AUTHORS

  - Namir Oues
  - Lexi Xu
  - BHM-Bob G  
+  - Debasish Mohanty


Please add your name at the end of the list.

orbeckst · 2025-04-03T00:48:30Z

testsuite/MDAnalysisTests/visualization/test_streamlines.py

+            [0.5, 0.5],
+            [1.5, 0.5],
+        ]
+    )


Even though I agree with @tylerjereddy , I am going to succumb to black's opinion here and just leave it in whichever way black wants it to be. Not worthwhile to mark it as an exception.

orbeckst · 2025-04-03T00:56:43Z

@TRY-ER sorry I said in my earlier review

We don't need a CHANGELOG entry for tests/internal refactoring.

I should have been more precise: we don't need a bullet point in CHANGELOG but we do want your name in there as a contributor. I realize that my earlier review may have come across as not valuing your contribution here. I am sorry for that. ALL contributions are important for the success of the project and we want to give credit to all contributors.

TRY-ER · 2025-04-03T05:09:19Z

Just for information! My name is Debasish Mohanty (In case my Github handle confuses anyone). [ I am proposing the MDAKit for polymers for GSoC 2025. ]

orbeckst

LGTM from my vantage point! Thanks. I am waiting for opinions from @tylerjereddy and @lilyminium .

lilyminium · 2025-04-03T10:49:57Z

testsuite/MDAnalysisTests/visualization/test_streamlines.py

+    square = [(0, 0), (1, 0), (1, 1), (0, 1)]
+    vertex_list = [square]
+    points = np.array([[1, 0.5]])  # exactly on the boundary


Thanks for adding this test! Just to be rigorous (and because I'm curious), what happens if vertex_list contains two adjacent squares both including the boundary that points lies on? Does _produce_list_indices_point_in_polygon_this_frame identify the point as being in both squares?

Yes, as it is implemented in the _produce_list_indices_point_in_polygon_this_frame function, the index of the point lying on the boundary of two of the adjacent squares will be included for both vertices. (identifying the point is on both squares)

I see, thanks. Sorry, I should have been clearer -- could you please update the test to include this test and make it clearer? Otherwise the PR looks good!

A test has been added to check if the indices of the shared points on adjacent squares return expected data from _produce_list_indices_point_in_polygon_this_frame or not.

lilyminium

LGTM -- thanks you @TRY-ER!

orbeckst · 2025-04-09T07:07:35Z

Thank you for your contribution @TRY-ER ! Congratulations on the merged PR 🎉 !

TRY-ER · 2025-04-09T11:34:19Z

Thank you @orbeckst , @lilyminium , @tylerjereddy for your kind guidance and support.

github-actions bot reviewed Mar 30, 2025

View reviewed changes

orbeckst requested changes Mar 30, 2025

View reviewed changes

orbeckst self-assigned this Mar 30, 2025

TRY-ER added 5 commits March 31, 2025 09:18

adding relevant tests and restructured 2D streamline for better test …

951418c

…coverage

fixing black formatting

4aa863a

fixing black linting manually

8786c18

fixed black linting for tests

ccb7c72

made some functions private and added author

a293f3a

TRY-ER force-pushed the visualize_test_coverage branch from 5c5e5a5 to a293f3a Compare March 31, 2025 03:49

lilyminium requested changes Mar 31, 2025

View reviewed changes

added two extra testcases! addressed other changes requested

2658006

tylerjereddy reviewed Mar 31, 2025

View reviewed changes

addressed commented changes

5d6d9df

TRY-ER requested review from lilyminium and orbeckst April 1, 2025 14:57

orbeckst requested changes Apr 3, 2025

View reviewed changes

updated author list and changelog

5607fbf

orbeckst approved these changes Apr 3, 2025

View reviewed changes

lilyminium reviewed Apr 3, 2025

View reviewed changes

minor comment update

77d08d1

TRY-ER requested a review from lilyminium April 4, 2025 09:32

added test for multiple points on multiple squares

26b7bc3

lilyminium approved these changes Apr 7, 2025

View reviewed changes

Merge branch 'develop' into visualize_test_coverage

afdd4f8

orbeckst merged commit 1cdb055 into MDAnalysis:develop Apr 9, 2025
23 of 24 checks passed

	expected = np.average(pts[[1, 3]], axis=0)
	expected = np.array([2., 2.])

		expected1 = np.average(pts[[0, 2]], axis=0)
		expected2 = np.average(pts[[1, 3, 4]], axis=0)

Conversation

TRY-ER commented Mar 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Developers Certificate of Origin

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

orbeckst commented Mar 30, 2025

Uh oh!

tylerjereddy commented Mar 31, 2025

Uh oh!

orbeckst commented Mar 31, 2025

Uh oh!

TRY-ER commented Mar 31, 2025

Uh oh!

lilyminium left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tylerjereddy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TRY-ER Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TRY-ER Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TRY-ER commented Apr 1, 2025

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TRY-ER commented Mar 30, 2025 •

edited

Loading

codecov bot commented Mar 30, 2025 •

edited

Loading

TRY-ER Apr 4, 2025 •

edited

Loading

TRY-ER Apr 1, 2025 •

edited

Loading

TRY-ER commented Apr 3, 2025 •

edited

Loading

TRY-ER commented Apr 9, 2025 •

edited

Loading