⛈️clean up, testing, documentation #23
Conversation
Reviewer's Guide by SourceryThis pull request focuses on cleaning up the code, adding tests, and improving documentation. The changes include refactoring code for better readability and maintainability, adding new test cases, and enhancing the documentation for various functions. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leaver2000 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 6 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
|
|
||
|
|
||
| FASTPATH: dict[str, Any] = {"__fastpath": True} | ||
|
|
||
|
|
||
| @broadcast_nz |
There was a problem hiding this comment.
question: Decorator placement might be unnecessary.
The @broadcast_nz decorator is used here, but it's not clear if the function parcel_mixing_ratio requires broadcasting. Please ensure that this decorator is necessary for this function.
| mixing ratio is calculated using the dewpoint upto the LCL (lifting condensation level) | ||
| and the temperature above the LCL. | ||
|
|
||
| Returns |
There was a problem hiding this comment.
issue: Incomplete return documentation.
The return section for the ccl function is marked as TODO. Please complete this section to provide clarity on what the function returns.
| @@ -203,7 +244,8 @@ | |||
| which_lfc: L["bottom", "top"] = "bottom", | |||
| which_el: L["bottom", "top"] = "top", | |||
| dewpoint_start: np.ndarray[shape[N], np.dtype[_T]] | None = None, | |||
| ) -> tuple[Vector1d[_T], Vector1d[_T]] | Vector1d[_T]: | |||
| ) -> tuple[Parcel[_T], Parcel[_T]] | Parcel[_T]: | |||
| r"""short cut for el, lfc, and el_lfc functions.""" | |||
There was a problem hiding this comment.
suggestion: Docstring is too brief.
The docstring for _el_lfc is very brief and does not provide enough context. Consider expanding it to explain the purpose and usage of the function.
| r"""short cut for el, lfc, and el_lfc functions.""" | |
| r""" | |
| Shortcut for calculating the Equilibrium Level (EL), | |
| Level of Free Convection (LFC), and a combined EL and LFC. | |
| Returns: | |
| A tuple of Parcel objects representing EL and LFC, or a single Parcel object. | |
| """ |
| @@ -564,15 +603,15 @@ | |||
| depth: Pascal[float] = 30000.0, | |||
| bottom: Pascal[float] | None = None, | |||
| ) -> tuple[np.ndarray, np.ndarray]: | |||
| r"""TODO ... | |||
| r"""Calculate most unstable CAPE and CIN. | |||
There was a problem hiding this comment.
issue: Incomplete docstring.
The docstring for most_unstable_cape_cin is marked as TODO. Please complete this section to provide clarity on the function's purpose and usage.
| # ............................................................................................... # | ||
| # nzthermo.core.ccl | ||
| # ............................................................................................... # | ||
| @pytest.mark.ccl |
There was a problem hiding this comment.
suggestion (testing): Missing edge case for test_ccl_metpy_regression
Consider adding a test case for when height is provided, even if it raises a NotImplementedError. This will ensure that the function behaves as expected when this parameter is used.
| @@ -1331,7 +1337,6 @@ | |||
| assert_array_equal( | |||
| most_unstable_parcel(P, T, Td, depth=depth), | |||
| most_unstable_parcel(np.broadcast_to(P, T.shape), T, Td, depth=depth), | |||
| err_msg="most_unstable_parcel failed to on broadcasted pressure input.", | |||
There was a problem hiding this comment.
issue (testing): Remove commented-out error messages
The commented-out error messages should be either re-enabled or removed. Keeping them commented out can lead to confusion and clutter in the test code.
| @@ -1356,15 +1361,11 @@ | |||
| # ............................................................................................... # | |||
| @pytest.mark.broadcasting | |||
| @pytest.mark.most_unstable_cape_cin | |||
| def test_most_unstable_cape_cin_broadcasting(): | |||
| @pytest.mark.parametrize("depth", [30000.0]) | |||
There was a problem hiding this comment.
suggestion (testing): Add more depth values for test_most_unstable_cape_cin_broadcasting
Consider adding more depth values to the parametrize decorator to ensure the function is tested across a wider range of scenarios.
| @pytest.mark.parametrize("depth", [30000.0]) | |
| @pytest.mark.parametrize("depth", [10000.0, 20000.0, 30000.0, 40000.0, 50000.0]) |
| """ | ||
|
|
||
| assert_allclose( | ||
| assert_array_equal( |
There was a problem hiding this comment.
issue (testing): Remove outdated comments
The outdated comments explaining the use of assert_allclose should be removed as they are no longer relevant.
| r = saturation_mixing_ratio( | ||
| pressure, | ||
| temperature, | ||
| out=saturation_mixing_ratio(pressure, dewpoint, where=where), | ||
| where=~where, | ||
| ) |
There was a problem hiding this comment.
issue (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
| temperature = temperature[sort][:, ::-1] | ||
| dewpoint = dewpoint[sort][:, ::-1] | ||
| else: | ||
| sort = np.arange(pressure.shape[0])[:, np.newaxis], p_sort | ||
| pressure = pressure[sort][:, ::-1] | ||
| temperature = temperature[sort][:, ::-1] | ||
| dewpoint = dewpoint[sort][:, ::-1] |
There was a problem hiding this comment.
issue (code-quality): Hoist repeated code outside conditional statement [×2] (hoist-statement-from-if)
Summary by Sourcery
This pull request introduces new functions for calculating meteorological parameters such as CCL, CAPE, and CIN. It also includes significant refactoring to improve code clarity and functionality, updates to documentation, and enhancements to the testing suite.
cclfunction to calculate the convective condensation level (CCL) and convective temperature.cape_cinfunction to calculate CAPE and CIN.most_unstable_cape_cinfunction to calculate the most unstable CAPE and CIN.parcel_mixing_ratiofunction to include detailed docstrings and improved parameter handling.mixed_layerfunction to handle broadcasting and added support for thewhereparameter.Vector1dandVector2dclasses withParcelandProfileclasses for better clarity and functionality.intersect_nzandzero_crossingsfunctions to returnProfileobjects instead ofVector2d.parcel_mixing_ratio,ccl,cape_cin,most_unstable_cape_cin, andmixed_layerfunctions.equivalent_potential_temperatureandtheta_wfunctions inlibthermo.cpp.cclfunction with various parameter combinations.mixed_layerto include cases with and without interpolation.assert_array_equalfor better accuracy and consistency.