Development #22
Development #22
Conversation
Reviewer's Guide by SourceryThis pull request introduces several new functions and updates existing ones with additional parameters, improved docstrings, and better type safety. It also adds new utility functions and updates imports. The tests have been expanded to cover the new functionality, and the code has been reformatted and cleaned up for better readability and performance. 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: 7 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 3 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.
| @@ -72,9 +74,10 @@ def downdraft_cape( | |||
| temperature: Kelvin[np.ndarray[shape[N, Z], np.dtype[_T]]], | |||
| dewpoint: Kelvin[np.ndarray[shape[N, Z], np.dtype[_T]]], | |||
| /, | |||
| *, | |||
There was a problem hiding this comment.
suggestion: Consider using keyword-only arguments.
The addition of *, makes all following arguments keyword-only. This is a good practice for clarity and to avoid potential bugs. Ensure this change is intentional and consistent with the rest of the codebase.
| where: np.ndarray[shape[N, Z], np.dtype[np.bool_]] | None = None, | ||
| ) -> np.ndarray[shape[N], np.dtype[_T]]: | ||
| """Calculate downward CAPE (DCAPE). | ||
| r"""Calculate downward CAPE (DCAPE). |
There was a problem hiding this comment.
question: Raw string literal usage.
Using a raw string literal (r""") for the docstring is unusual unless there are backslashes in the text. Ensure this is necessary.
| Temperature (K) at the levels given by `pressure` | ||
| dewpoint : `array_like[[N, Z], floating]` | ||
| Dewpoint (K) at the levels given by `pressure` | ||
| TODO : ... |
There was a problem hiding this comment.
issue: Incomplete docstring.
The docstring for the ccl function is incomplete. Ensure that the TODOs are addressed before merging.
| @@ -289,6 +356,26 @@ | |||
| which_el: L["bottom", "top"] = "top", | |||
| dewpoint_start: np.ndarray[shape[N], np.dtype[_T]] | None = None, | |||
| ) -> tuple[Vector1d[_T], Vector1d[_T]]: | |||
| r"""TODO ... | |||
There was a problem hiding this comment.
issue: Incomplete docstring.
The docstring for the el_lfc function is incomplete. Ensure that the TODOs are addressed before merging.
| @@ -346,6 +453,26 @@ | |||
| Kelvin[np.ndarray[shape[N], np.dtype[_T]]], | |||
| np.ndarray[shape[N, Z], np.dtype[np.intp]], | |||
| ]: | |||
| r"""Calculate the most unstable parcel profile. | |||
There was a problem hiding this comment.
issue: Incomplete docstring.
The docstring for the most_unstable_parcel function is incomplete. Ensure that the TODOs are addressed before merging.
| temperature: Kelvin[np.ndarray[shape[N, Z], np.dtype[np.floating[Any]]]], | ||
| dewpoint: Kelvin[np.ndarray[shape[N, Z], np.dtype[np.floating[Any]]]], | ||
| /, | ||
| *, | ||
| where: np.ndarray[shape[N], np.dtype[np.bool_]] | None = ..., | ||
| ) -> tuple[ | ||
| Pascal[np.ndarray[shape[N, Z], np.dtype[T]]], | ||
| Pascal[pressure_vector[shape[N, Z], np.dtype[T]]], |
There was a problem hiding this comment.
suggestion: Type hinting for pressure_vector.
The use of pressure_vector in type hinting improves code clarity. Ensure this is consistent across the codebase.
| Pascal[pressure_vector[shape[N, Z], np.dtype[T]]], | |
| Pascal[np.ndarray[shape[N, Z], np.dtype[T]]], | |
| Pascal[pressure_vector[shape[N, Z], np.dtype[T]]], |
| T fabs[T](T) noexcept | ||
| bint isnan(double) noexcept |
There was a problem hiding this comment.
suggestion (performance): Use of C++ standard library functions.
The inclusion of fabs and isnan from the C++ standard library is a good practice for performance. Ensure these are used consistently.
| T fabs[T](T) noexcept | |
| bint isnan(double) noexcept | |
| cdef extern from "<cmath>" namespace "std" nogil: | |
| T fabs[T](T) noexcept | |
| bint isnan(double) noexcept | |
| cdef inline T abs_value[T](T x) nogil: | |
| return fabs(x) | |
| cdef inline bint is_nan(double x) nogil: | |
| return isnan(x) |
| # nzthermo.core.most_unstable_parcel | ||
| # nzthermo.core.cape_cin | ||
| # ............................................................................................... # | ||
| @pytest.mark.cape_cin |
There was a problem hiding this comment.
suggestion (testing): Missing edge case for cape_cin function
Consider adding a test case for the cape_cin function where the input arrays contain NaN values. This will help ensure that the function handles missing data gracefully.
| @pytest.mark.cape_cin | |
| @pytest.mark.cape_cin | |
| @pytest.mark.broadcasting | |
| def test_cape_cin_broadcasting_with_nan(): | |
| input_data_with_nan = np.array([[300.0, np.nan], [np.nan, 310.0]]) | |
| result = cape_cin(input_data_with_nan) | |
| assert not np.isnan(result).all() |
| # nzthermo.core.most_unstable_cape_cin | ||
| # ............................................................................................... # | ||
| @pytest.mark.broadcasting | ||
| @pytest.mark.most_unstable_cape_cin |
There was a problem hiding this comment.
suggestion (testing): Missing edge case for most_unstable_cape_cin function
Consider adding a test case for the most_unstable_cape_cin function where the input arrays contain NaN values. This will help ensure that the function handles missing data gracefully.
| @pytest.mark.most_unstable_cape_cin | |
| @pytest.mark.most_unstable_cape_cin | |
| def test_most_unstable_cape_cin_with_nan(): | |
| input_data_with_nan = np.array([[np.nan, 300], [250, np.nan]]) | |
| result = most_unstable_cape_cin(input_data_with_nan) | |
| assert np.isnan(result).any() |
| Td, | ||
| depth=30000.0, | ||
| ), | ||
| @pytest.mark.mixed_layer_cape_cin |
There was a problem hiding this comment.
suggestion (testing): Missing edge case for mixed_layer_cape_cin function
Consider adding a test case for the mixed_layer_cape_cin function where the input arrays contain NaN values. This will help ensure that the function handles missing data gracefully.
| @pytest.mark.mixed_layer_cape_cin | |
| @pytest.mark.mixed_layer_cape_cin | |
| def test_mixed_layer_cape_cin_with_nan() -> None: | |
| """ | |
| Test mixed_layer_cape_cin function with NaN values in input arrays. | |
| """ | |
| Td = np.array([np.nan, 10.0, 20.0]) | |
| depth = 30000.0 | |
| result = mixed_layer_cape_cin(Td, depth) | |
| assert np.isnan(result).any() |
Summary by Sourcery
This pull request introduces new features for calculating mixed-layer CAPE and CIN, enhances type annotations and docstrings for better clarity, and adds comprehensive tests to ensure functionality and regression coverage. Additionally, it includes code refactoring and cleanup to improve maintainability.
mixed_layer_cape_cinandmixed_parcelfunctions to calculate mixed-layer CAPE and CIN, and mixed parcel properties respectively.pressure_vectorclass to include additional methods for pressure comparisons and broadcasting support.lclclass and related functions with better initialization and method definitions.mixed_layer_cape_cinandmixed_parcelfunctions to ensure correct functionality and regression testing.cape_cin,most_unstable_cape_cin, andmixed_parcelfunctions to validate broadcasting behavior.