Skip to content
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

Improve Test Coverage and Refactor Test for mslib.utils.coordinate #2596

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

saiabhinav001
Copy link

This PR enhances the test suite for the mslib.utils.coordinate module.

Key Updates:

  • Refactored and added new test cases for functions in the coordinate module.
  • Used pytest.mark.parametrize to simplify and expand parameterized testing.
  • Improved edge case coverage for distance, projection, and angle calculations.
  • Enhanced test readability and maintainability with detailed docstrings.
  • Validated linear and great circle path generation for lat/lon points.

Purpose of PR?:

Fixes #

Does this PR introduce a breaking change?
This PR does not introduce any breaking changes.

If the changes in this PR are manually verified, list down the scenarios covered::

If manually verified, the following scenarios were tested:

  1. Distance calculations across diverse lat/lon coordinates.
  2. Angle normalization and edge cases, including negative and large angles.
  3. Path generation for both linear and great circle connections with varied numpoints.
  4. Projection parameter retrieval for valid and invalid projection strings.

Additional information for reviewer? :
This PR is an independent contribution and does not depend on or continue from previous PRs.

Does this PR results in some Documentation changes?
No documentation changes are required.

Checklist:

  • Bug fix. Fixes
  • New feature (Non-API breaking changes that adds functionality)
  • PR Title follows the convention of <type>: <subject>
  • Commit has unit tests

Add comprehensive test suite for `mslib.utils.coordinate` module  

- Refactored and added new test cases for functions in `coordinate` module.  
- Used `pytest.mark.parametrize` for parameterized testing.  
- Improved edge case coverage for distance, projection, and angle calculations.  
- Enhanced test readability and maintainability with detailed docstrings.  
- Validated linear and great circle path generation for lat/lon points.
@ReimarBauer ReimarBauer requested review from matrss and joernu76 January 6, 2025 07:42

@pytest.mark.parametrize("ref_lats, ref_lons, numpoints", [
([0, 10], [0, 0], 2),
([0, 10], [0, 0], 3),
Copy link
Member

@ReimarBauer ReimarBauer Jan 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the second one had before ([0, 0], [0, 10] ,3)

assert all(np.asarray(lons) == [0, 5, 10])
numpoints=numpoints, connection="greatcircle")
assert len(lats) == numpoints
assert len(lons) == numpoints
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also check for the expected value, like in assert all(np.asarray(lons) == [0, 5, 10])

Copy link
Member

@ReimarBauer ReimarBauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @saiabhinav001
that makes it much better readable, thanks!

while the module we test here has no class definition we use a class level organization as namespace grouping. When test_linear maybe get named test_latlon_points_linear and the other test_latlon_points_greatcircle we maybe can remove all the grouping class names, what do you think?

Copy link
Author

@saiabhinav001 saiabhinav001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ReimarBauer
That's a great suggestion! Renaming the test functions makes them more descriptive, and removing the class-level grouping could simplify the structure.

I’ve updated the code by renaming test_linear to test_latlon_points_linear and test_greatcircle to test_latlon_points_greatcircle. I also removed the class-level groupings for simplicity and better readability. Let me know if there’s anything else to adjust!

@ReimarBauer
Copy link
Member

Hi @saiabhinav001 Maybe a push to the repo is missing? I do see the class names here and also there is a lint failure.

@ReimarBauer ReimarBauer changed the title Improve Test Coverage and Refactor for mslib.utils.coordinate Improve Test Coverage and Refactor Test for mslib.utils.coordinate Jan 9, 2025
Copy link
Author

@saiabhinav001 saiabhinav001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ReimarBauer, I ensured there were no lint failures and pushed the repo.

@ReimarBauer
Copy link
Member

ReimarBauer commented Jan 11, 2025

@saiabhinav001 why do you have closed it?

@saiabhinav001 saiabhinav001 reopened this Jan 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants