Development #17
Development #17
Conversation
Reviewer's Guide by SourceryThis pull request refactors and adds new test functions, updates type annotations, and optimizes existing functions for better performance and readability. It also deletes unused and redundant files to clean up the codebase. 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: 5 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.
| _DualArrayLike = ( | ||
| SupportsArray[_Dtype_t] | NestedSequence[SupportsArray[_Dtype_t]] | _T | NestedSequence[_T] |
There was a problem hiding this comment.
suggestion: Consider using a type alias for readability.
The type definition for _DualArrayLike is quite complex. Consider using a type alias to improve readability and maintainability.
| _DualArrayLike = ( | |
| SupportsArray[_Dtype_t] | NestedSequence[SupportsArray[_Dtype_t]] | _T | NestedSequence[_T] | |
| _SupportsArrayOrNested = SupportsArray[_Dtype_t] | NestedSequence[SupportsArray[_Dtype_t]] | |
| _TOrNested = _T | NestedSequence[_T] | |
| _DualArrayLike = _SupportsArrayOrNested | _TOrNested |
| if broadcasted := pressure.shape == temperature.shape: | ||
| p_layer, t_layer, td_layer = np.where( | ||
| mask[newaxis, :, :], [pressure, temperature, dewpoint], NaN | ||
| ) | ||
| else: | ||
| p_layer = np.where(mask, pressure, NaN) | ||
| t_layer, td_layer = np.where(mask[newaxis, :, :], [temperature, dewpoint], NaN) |
There was a problem hiding this comment.
suggestion: Consider simplifying the conditional logic.
The conditional logic for broadcasted is a bit complex. Consider simplifying or breaking it down for better readability.
| if broadcasted := pressure.shape == temperature.shape: | |
| p_layer, t_layer, td_layer = np.where( | |
| mask[newaxis, :, :], [pressure, temperature, dewpoint], NaN | |
| ) | |
| else: | |
| p_layer = np.where(mask, pressure, NaN) | |
| t_layer, td_layer = np.where(mask[newaxis, :, :], [temperature, dewpoint], NaN) | |
| broadcasted = pressure.shape == temperature.shape | |
| if broadcasted: | |
| p_layer, t_layer, td_layer = np.where( | |
| mask[newaxis, :, :], [pressure, temperature, dewpoint], NaN | |
| ) | |
| else: | |
| p_layer = np.where(mask, pressure, NaN) | |
| t_layer, td_layer = np.where(mask[newaxis, :, :], [temperature, dewpoint], NaN) |
| @@ -170,7 +164,7 @@ | |||
| pressure, temperature, dewpoint | |||
| ) | |||
|
|
|||
| N, Z = temperature.shape | |||
| N = temperature.shape[0] | |||
There was a problem hiding this comment.
nitpick: Remove unused variable assignment.
The variable N is assigned but not used in the subsequent code. Consider removing it to avoid confusion.
| x_full[nx, z0] = x | ||
| y_full[nx, z0] = y |
There was a problem hiding this comment.
issue (bug_risk): Check for potential index out-of-bounds.
Ensure that nx and z0 indices are within the bounds of x_full and y_full arrays to avoid potential index out-of-bounds errors.
| @@ -18,43 +18,43 @@ _float = TypeVar("_float", np.float32, np.float64) | |||
| OPENMP_ENABLED: bool | |||
|
|
|||
| @overload | |||
| def moist_lapse[T: np.float_]( | |||
| def moist_lapse[T: np.floating[Any]]( | |||
There was a problem hiding this comment.
suggestion: Consider adding type constraints for dtype.
The dtype parameter accepts type[T | float] | L["float32", "float64"] | None. Consider adding type constraints to ensure it aligns with the expected floating-point types.
| def moist_lapse[T: np.floating[Any]]( | |
| def moist_lapse[T: np.floating[Any]]( | |
| pressure: Pascal[np.ndarray[shape[N], np.dtype[T]]], | |
| temperature: Kelvin[np.ndarray[shape[N], np.dtype[np.floating[Any]]]], | |
| reference_pressure: Pascal[np.ndarray[shape[N], np.dtype[np.floating[Any]]]], | |
| *, | |
| dtype: type[T] | type[float] | Literal["float32", "float64"] | None = ..., | |
| ) -> Kelvin[np.ndarray[shape[N], np.dtype[T]]]: ... |
| DCAPE = Rd * F.nantrapz(p_vt - e_vt, np.log(pressure), axis=1) | ||
|
|
||
| dcape = -(Rd * F.nantrapz(delta, np.log(pressure), axis=1)) | ||
|
|
||
| return dcape | ||
| return DCAPE |
There was a problem hiding this comment.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
| DCAPE = Rd * F.nantrapz(p_vt - e_vt, np.log(pressure), axis=1) | |
| dcape = -(Rd * F.nantrapz(delta, np.log(pressure), axis=1)) | |
| return dcape | |
| return DCAPE | |
| return Rd * F.nantrapz(p_vt - e_vt, np.log(pressure), axis=1) |
| if len(args) == 1: | ||
| return args[0] | ||
|
|
||
| return tuple(values) | ||
| return args |
There was a problem hiding this comment.
suggestion (code-quality): We've found these issues:
- Lift code into else after jump in control flow (
reintroduce-else) - Replace if statement with if expression (
assign-if-exp)
| if len(args) == 1: | |
| return args[0] | |
| return tuple(values) | |
| return args | |
| return args[0] if len(args) == 1 else args |
| try: | ||
| import pint | ||
| except ImportError: | ||
| pint = None | ||
| except AttributeError: | ||
| raise ImportError( | ||
| "The environment has a version mismatch with pint and numpy. " | ||
| "Upgrade pint to the latest version." | ||
| ) |
There was a problem hiding this comment.
issue (code-quality): Explicitly raise from a previous error (raise-from-previous-error)
| if isinstance(x, pint.Quantity): | ||
| return x.to(unit).magnitude | ||
| return np.asarray(x) |
There was a problem hiding this comment.
suggestion (code-quality): We've found these issues:
- Lift code into else after jump in control flow (
reintroduce-else) - Replace if statement with if expression (
assign-if-exp)
| if isinstance(x, pint.Quantity): | |
| return x.to(unit).magnitude | |
| return np.asarray(x) | |
| return x.to(unit).magnitude if isinstance(x, pint.Quantity) else np.asarray(x) |
| if method == "__call__": | ||
| return ufunc(self.pressure) | ||
|
|
||
| return NotImplemented |
There was a problem hiding this comment.
suggestion (code-quality): We've found these issues:
- Lift code into else after jump in control flow (
reintroduce-else) - Replace if statement with if expression (
assign-if-exp)
| if method == "__call__": | |
| return ufunc(self.pressure) | |
| return NotImplemented | |
| return ufunc(self.pressure) if method == "__call__" else NotImplemented |
Summary by Sourcery
This pull request includes significant refactoring and enhancements to the core functions and tests in the project. Key changes include improved handling of NaN values, conditional checks, and type safety. New test cases have been added to ensure robustness, and obsolete files have been removed.
assert_nanfunction to handle both single and multiple NaN checks.pressure_levelsfunction to generate pressure levels array.moist_lapsetests with new test cases for element-wise, broadcasting, and matrix operations.downdraft_capefunction to handle conditional parcel calculations and NaN values.elandlfcfunctions to improve handling of NaN values and conditional checks.PVectorNdclass to include new methods for conditional checks and handling NaN values.moist_lapse,downdraft_cape,el, andlfcfunctions.data.json,notebook.ipynb,nb.ipynb,moist_lapse_test.py,libcore_test.py, anddcape_test.py.