Development #21
Development #21
Conversation
Reviewer's Guide by SourceryThis pull request introduces significant refactoring and new features to the nzthermo library. Key changes include the addition of the pressure_vector class, new functions for calculating mixed layer and most unstable parcel properties, and updates to existing functions to support broadcasting and additional parameters. The tests have been updated to cover these new scenarios and ensure compatibility with the refactored code. 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: 6 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.
| # pressure = pressure.where(pressure >= p_top[:, newaxis], NaN) | ||
| pressure = pressure.where(pressure.is_below(p_top[:, newaxis], close=True), NaN) |
There was a problem hiding this comment.
nitpick: Commented-out code
Consider removing the commented-out line if it is no longer needed. Keeping commented-out code can lead to confusion and clutter.
| temperature, | ||
| "increasing", | ||
| log_x=True, | ||
| ).where_above(LCL) | ||
|
|
||
| is_lcl = (no_lfc := LFC.is_nan().all(axis=1, keepdims=True)) & greater_or_close( | ||
| no_lfc = LFC.is_nan().all(Axis.Z, out=np.empty((N, 1), dtype=np.bool_), keepdims=True) |
There was a problem hiding this comment.
suggestion (performance): Potential performance issue with np.empty
Using np.empty without initializing values can lead to unpredictable behavior if the array is used before being fully populated. Consider using np.zeros or np.full for safer initialization.
| no_lfc = LFC.is_nan().all(Axis.Z, out=np.empty((N, 1), dtype=np.bool_), keepdims=True) | |
| no_lfc = LFC.is_nan().all(Axis.Z, out=np.zeros((N, 1), dtype=np.bool_), keepdims=True) |
| np.ndarray pressure, | ||
| np.ndarray temperature, | ||
| np.ndarray dewpoint, | ||
| def broadcast_and_nanmask( |
There was a problem hiding this comment.
suggestion: Function documentation improvement
The docstring for the 'broadcast_and_nanmask' function is detailed but could benefit from a clearer explanation of the 'where' parameter and its expected behavior.
| def broadcast_and_nanmask( | |
| def broadcast_and_nanmask( | |
| np.ndarray pressure, | |
| np.ndarray temperature, | |
| np.ndarray dewpoint, | |
| where=None | |
| ): |
| pressure = pressure.reshape(1, -1) | ||
| mode = BROADCAST | ||
| else: | ||
| mode = MATRIX | ||
|
|
||
| if where is not None: |
There was a problem hiding this comment.
issue: Unsupported 'where' argument
The 'where' argument is not supported in the 'moist_lapse' function. Consider either implementing support for it or removing the parameter to avoid confusion.
|
|
||
| elif pint is not None: | ||
|
|
||
| def magnitude(x, unit): | ||
| if isinstance(x, pint.Quantity): | ||
| return x.to(unit).magnitude | ||
| x = x.to(unit).magnitude |
There was a problem hiding this comment.
issue: Inconsistent return type
The 'magnitude' function should consistently return a numpy array. Ensure that the return type is always np.asarray(x) to avoid potential type issues.
| @@ -301,8 +312,17 @@ def test_moist_lapse(dtype): | |||
| # ............................................................................................... # | |||
| # nzthermo._core.parcel_profile | |||
| # ............................................................................................... # | |||
| @pytest.mark.broadcasting | |||
There was a problem hiding this comment.
suggestion (testing): Add edge case for empty input arrays
Consider adding a test case to handle empty input arrays for the test_parcel_profile_broadcasting function. This will ensure that the function can gracefully handle scenarios where no data is provided.
| @pytest.mark.broadcasting | |
| @pytest.mark.broadcasting | |
| @pytest.mark.parametrize("input_data", [[], np.array([])]) | |
| def test_parcel_profile_broadcasting(input_data) -> None: |
| ) | ||
|
|
||
|
|
||
| @pytest.mark.regression |
There was a problem hiding this comment.
suggestion (testing): Add edge case for extreme values
Consider adding a test case to handle extreme values (e.g., very high or very low temperatures and pressures) for the test_parcel_profile_metpy_regression function. This will ensure that the function can handle a wide range of input values.
| @pytest.mark.regression | |
| @pytest.mark.regression | |
| @pytest.mark.parametrize("depth, temperature, pressure", [ | |
| (30000.0, 273.15, 1013.25), # Normal conditions | |
| (30000.0, 373.15, 500.0), # High temperature, low pressure | |
| (30000.0, 173.15, 2000.0), # Low temperature, high pressure | |
| ]) |
| @@ -1055,6 +1089,15 @@ | |||
| # ............................................................................................... # | |||
| # nzthermo.core.lfc | |||
| # ............................................................................................... # | |||
| @pytest.mark.broadcasting | |||
| @pytest.mark.lfc | |||
There was a problem hiding this comment.
suggestion (testing): Add edge case for empty input arrays
Consider adding a test case to handle empty input arrays for the test_lfc_broadcasting function. This will ensure that the function can gracefully handle scenarios where no data is provided.
| @pytest.mark.lfc | |
| @pytest.mark.lfc | |
| def test_lfc_broadcasting_empty_input() -> None: | |
| empty_array = np.array([]) | |
| assert_array_equal(lfc(empty_array), empty_array) |
Summary by Sourcery
This pull request introduces several new features for atmospheric calculations, including functions for parcel mixing ratio, most unstable parcel identification, mixed layer calculations, and CAPE/CIN computations. It also includes significant enhancements to existing functions, adding support for conditional calculations and improving broadcasting and masking capabilities. Comprehensive tests and documentation updates are provided to ensure robustness and clarity.
parcel_mixing_ratiofunction to calculate the mixing ratio of a parcel based on pressure, temperature, and dewpoint.most_unstable_parcelfunction to identify the most unstable parcel in a given atmospheric profile.mixed_layerfunction to calculate the mixed layer temperature and dewpoint.most_unstable_cape_cinfunction to compute CAPE and CIN for the most unstable parcel.downdraft_cape,ccl,el, andlfcfunctions to include optionalwhereparameter for conditional calculations.broadcast_and_nanmaskfunction to handle broadcasting and masking of input arrays more efficiently.parcel_profile_with_lclto include broadcasting and masking capabilities.moist_lapsefunction to supportwhereparameter for conditional calculations.pressure_vectorclass to include methods for checking and masking pressure levels.equivalent_potential_temperatureand other functions to include detailed parameter descriptions and examples.parcel_mixing_ratio,most_unstable_parcel,mixed_layer, andmost_unstable_cape_cinfunctions.