Expand test suite across geo, coordinates, and distributions#366
Expand test suite across geo, coordinates, and distributions#366lispandfound wants to merge 3 commits intotype_checkingfrom
Conversation
Changes: - tests/test_cli.py: Re-enabled previously skipped docstring tests. - tests/test_coordinates.py: Added validation for NZTM/WGS conversions, distance calculations, and bearing transformations. - tests/test_distributions.py: New test file covering truncated normal, log-normal, and Weibull distributions using Hypothesis. - tests/test_geo.py: Expanded coverage for transformation matrices (gen_mat), coordinate rotations, and cross/along-track distances. - tests/test_grid.py: Added error handling for missing resolution parameters in patchgrids. - tests/test_xyts.py: Added tests for meta-only access restrictions in XYTSFile.
Summary of ChangesHello @lispandfound, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the test suite across several core modules, including command-line interface utilities, geographical coordinate transformations, statistical distributions, and grid generation. The primary goal is to improve the robustness and reliability of the codebase by adding new tests for existing functionalities, re-enabling previously skipped tests, and introducing property-based testing for statistical distributions. This ensures that various calculations and data access patterns behave as expected, especially under edge conditions and when dealing with meta-only data structures. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly expands the test suite, which is a great improvement. The new tests cover various functionalities in geo, coordinates, and distributions, including nominal cases, edge cases, and error handling. The use of Hypothesis for property-based testing is particularly good. I've made a few suggestions to improve the maintainability and consistency of the new test code, mainly by reducing duplication through parametrization and fixtures, and by adding a missing type hint. Overall, this is a solid contribution to improving the project's test coverage.
| def test_wgs_depth_to_nztm_invalid_coordinates() -> None: | ||
| with pytest.raises( | ||
| ValueError, | ||
| match=re.escape( | ||
| "Latitude and longitude coordinates given are invalid (did you input lon, lat instead of lat, lon?)" | ||
| ), | ||
| ): | ||
| coordinates.wgs_depth_to_nztm(np.array([-180.0, 0.0])) | ||
|
|
||
| with pytest.raises( | ||
| ValueError, | ||
| match=re.escape( | ||
| "Latitude and longitude coordinates given are invalid (did you input lon, lat instead of lat, lon?)" | ||
| ), | ||
| ): | ||
| coordinates.wgs_depth_to_nztm(np.array([np.nan, np.nan])) |
There was a problem hiding this comment.
The two test cases for invalid coordinates can be combined into a single parametrized test for better conciseness and maintainability. This avoids repeating the pytest.raises context manager.
| def test_wgs_depth_to_nztm_invalid_coordinates() -> None: | |
| with pytest.raises( | |
| ValueError, | |
| match=re.escape( | |
| "Latitude and longitude coordinates given are invalid (did you input lon, lat instead of lat, lon?)" | |
| ), | |
| ): | |
| coordinates.wgs_depth_to_nztm(np.array([-180.0, 0.0])) | |
| with pytest.raises( | |
| ValueError, | |
| match=re.escape( | |
| "Latitude and longitude coordinates given are invalid (did you input lon, lat instead of lat, lon?)" | |
| ), | |
| ): | |
| coordinates.wgs_depth_to_nztm(np.array([np.nan, np.nan])) | |
| @pytest.mark.parametrize( | |
| "invalid_coords", | |
| [ | |
| np.array([-180.0, 0.0]), | |
| np.array([np.nan, np.nan]), | |
| ], | |
| ) | |
| def test_wgs_depth_to_nztm_invalid_coordinates(invalid_coords: np.ndarray) -> None: | |
| with pytest.raises( | |
| ValueError, | |
| match=re.escape( | |
| "Latitude and longitude coordinates given are invalid (did you input lon, lat instead of lat, lon?)" | |
| ), | |
| ): | |
| coordinates.wgs_depth_to_nztm(invalid_coords) |
| def test_nztm_wgs_depth_invalid_coordinates() -> None: | ||
| with pytest.raises( | ||
| ValueError, | ||
| match=re.escape( | ||
| "NZTM coordinates given are invalid (did you input x, y instead of y, x?)" | ||
| ), | ||
| ): | ||
| coordinates.nztm_to_wgs_depth(np.array([1e10, 1e10])) | ||
| with pytest.raises( | ||
| ValueError, | ||
| match=re.escape( | ||
| "NZTM coordinates given are invalid (did you input x, y instead of y, x?)" | ||
| ), | ||
| ): | ||
| coordinates.nztm_to_wgs_depth(np.array([np.nan, np.nan])) |
There was a problem hiding this comment.
Similar to the previous test, these two test cases for invalid NZTM coordinates can be combined into a single parametrized test to improve code conciseness and reduce duplication.
| def test_nztm_wgs_depth_invalid_coordinates() -> None: | |
| with pytest.raises( | |
| ValueError, | |
| match=re.escape( | |
| "NZTM coordinates given are invalid (did you input x, y instead of y, x?)" | |
| ), | |
| ): | |
| coordinates.nztm_to_wgs_depth(np.array([1e10, 1e10])) | |
| with pytest.raises( | |
| ValueError, | |
| match=re.escape( | |
| "NZTM coordinates given are invalid (did you input x, y instead of y, x?)" | |
| ), | |
| ): | |
| coordinates.nztm_to_wgs_depth(np.array([np.nan, np.nan])) | |
| @pytest.mark.parametrize( | |
| "invalid_coords", | |
| [ | |
| np.array([1e10, 1e10]), | |
| np.array([np.nan, np.nan]), | |
| ], | |
| ) | |
| def test_nztm_wgs_depth_invalid_coordinates(invalid_coords: np.ndarray) -> None: | |
| with pytest.raises( | |
| ValueError, | |
| match=re.escape( | |
| "NZTM coordinates given are invalid (did you input x, y instead of y, x?)" | |
| ), | |
| ): | |
| coordinates.nztm_to_wgs_depth(invalid_coords) |
| seed=st.integers(0, 1_000_000), | ||
| ) | ||
| @settings(max_examples=20) | ||
| def test_truncated_log_normal_vectorized(mean: float, std_dev: float, seed: int): |
There was a problem hiding this comment.
For consistency with other test functions in this file and for better type safety, please add the -> None return type hint to this function signature.
| def test_truncated_log_normal_vectorized(mean: float, std_dev: float, seed: int): | |
| def test_truncated_log_normal_vectorized(mean: float, std_dev: float, seed: int) -> None: |
| def test_tslice_get_meta_only() -> None: | ||
| """Test that AttributeError is raised when tslice_get is called on meta-only instance.""" | ||
| test_file = Path(__file__).parent / "sample1" / "xyts.e3d" | ||
| if not test_file.exists(): | ||
| pytest.skip("Test file not available") | ||
|
|
||
| xyts_file = xyts.XYTSFile(str(test_file), meta_only=True) | ||
|
|
||
| with pytest.raises( | ||
| AttributeError, match="The data attribute must be set to use `tslice_get`" | ||
| ): | ||
| xyts_file.tslice_get(10, comp=xyts.Component.MAGNITUDE) | ||
|
|
||
|
|
||
| def test_pgv_meta_only() -> None: | ||
| """Test that AttributeError is raised when pgv is called on meta-only instance.""" | ||
| test_file = Path(__file__).parent / "sample1" / "xyts.e3d" | ||
| if not test_file.exists(): | ||
| pytest.skip("Test file not available") | ||
|
|
||
| xyts_file = xyts.XYTSFile(str(test_file), meta_only=True) | ||
|
|
||
| with pytest.raises( | ||
| AttributeError, match="The data and ll_map attributes must be set to use `pgv`" | ||
| ): | ||
| xyts_file.pgv() |
There was a problem hiding this comment.
The setup logic for creating a meta-only XYTSFile instance is duplicated in test_tslice_get_meta_only and test_pgv_meta_only. To improve code reuse and follow pytest best practices, you can extract this setup into a dedicated fixture.
Here's an example of how you could refactor this:
@pytest.fixture
def meta_only_xyts_file() -> xyts.XYTSFile:
"""Provide a meta-only XYTSFile instance."""
test_file = Path(__file__).parent / "sample1" / "xyts.e3d"
if not test_file.exists():
pytest.skip("Test file not available")
return xyts.XYTSFile(str(test_file), meta_only=True)
def test_tslice_get_meta_only(meta_only_xyts_file: xyts.XYTSFile) -> None:
"""Test that AttributeError is raised when tslice_get is called on meta-only instance."""
with pytest.raises(
AttributeError, match="The data attribute must be set to use `tslice_get`"
):
meta_only_xyts_file.tslice_get(10, comp=xyts.Component.MAGNITUDE)
def test_pgv_meta_only(meta_only_xyts_file: xyts.XYTSFile) -> None:
"""Test that AttributeError is raised when pgv is called on meta-only instance."""
with pytest.raises(
AttributeError, match="The data and ll_map attributes must be set to use `pgv`"
):
meta_only_xyts_file.pgv()
1 participant
distance calculations, and bearing transformations.
normal, log-normal, and Weibull distributions using Hypothesis.
(gen_mat), coordinate rotations, and cross/along-track distances.
parameters in patchgrids.
in XYTSFile.