⛈️ clean up #20
⛈️ clean up #20
Conversation
Reviewer's Guide by SourceryThis pull request focuses on cleaning up the codebase by simplifying function and constructor signatures in the Cython file and enhancing type annotations in the README. Additionally, a new section for Notebooks has been added to the README to provide users with more resources. 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: all looks good
- 🟢 Complexity: all looks good
- 🟡 Documentation: 2 issues found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| @@ -39,17 +39,17 @@ cdef extern from "wind.cpp" namespace "libthermo" nogil: | |||
| cdef cppclass wind_components[T]: | |||
| T u, v | |||
| wind_components() noexcept | |||
| wind_components(T d, T m) noexcept | |||
There was a problem hiding this comment.
suggestion: Consider adding parameter names for clarity.
Removing parameter names from the function signature can make the code less readable and harder to understand, especially for new developers or when revisiting the code after some time. Consider keeping the parameter names for better clarity.
| wind_components(T d, T m) noexcept | |
| wind_components(T d, T m) noexcept |
| @@ -39,17 +39,17 @@ | |||
| cdef cppclass wind_components[T]: | |||
| T u, v | |||
| wind_components() noexcept | |||
| wind_components(T d, T m) noexcept | |||
| wind_components(wind_vector[T] dm) noexcept | |||
There was a problem hiding this comment.
suggestion: Consider adding parameter names for clarity.
Similar to the previous comment, removing parameter names can reduce code readability. Including parameter names helps in understanding the purpose of each parameter.
| wind_components(wind_vector[T] dm) noexcept | |
| wind_components(T d, T m) noexcept | |
| wind_components(wind_vector[T] dm) noexcept |
|
|
||
| cdef cppclass wind_vector[T]: | ||
| T direction, magnitude | ||
| wind_vector() noexcept | ||
| wind_vector(T d, T s) noexcept |
There was a problem hiding this comment.
suggestion: Consider adding parameter names for clarity.
Parameter names provide context and improve the readability of the code. It is generally a good practice to include them in function signatures.
| wind_vector(T d, T s) noexcept | |
| wind_vector(T direction, T magnitude) noexcept |
|
|
||
| cdef cppclass wind_vector[T]: | ||
| T direction, magnitude | ||
| wind_vector() noexcept | ||
| wind_vector(T d, T s) noexcept | ||
| wind_vector(wind_components[T] uv) noexcept |
There was a problem hiding this comment.
suggestion: Consider adding parameter names for clarity.
Including parameter names in function signatures helps in understanding the code better and makes it more maintainable.
| wind_vector(wind_components[T] uv) noexcept | |
| wind_vector(T direction, T magnitude) noexcept | |
| wind_vector(wind_components[T] uv) noexcept |
|
|
||
| T wind_direction[T](T u, T v) noexcept |
There was a problem hiding this comment.
suggestion: Consider adding parameter names for clarity.
Parameter names in function signatures provide valuable context and improve code readability. It is recommended to keep them for better understanding.
| T wind_direction[T](T u, T v) noexcept | |
| T wind_direction[T](T u, T v) noexcept |
|
|
||
| T wind_direction[T](T u, T v) noexcept | ||
| T wind_magnitude[T](T u, T v) noexcept |
There was a problem hiding this comment.
suggestion: Consider adding parameter names for clarity.
Removing parameter names can make the code less intuitive. Including them helps in understanding the function's purpose and usage.
| T wind_magnitude[T](T u, T v) noexcept | |
| T wind_direction[T](T u, T v) noexcept | |
| T wind_magnitude[T](T u, T v) noexcept |
| @@ -13,8 +13,8 @@ verbose usage of type annotations. For example, the `parcel_profile` function is | |||
| ```python | |||
| def parcel_profile( | |||
| pressure: Pascal[np.ndarray[shape[Z], np.dtype[T]] | np.ndarray[shape[N, Z], np.dtype[T]]], | |||
| temperature: Kelvin[np.ndarray[shape[N], np.dtype[np.float_]]], | |||
| dewpoint: Kelvin[np.ndarray[shape[N], np.dtype[np.float_]]], | |||
| temperature: Kelvin[np.ndarray[shape[N], np.dtype[np.floating[Any]]]], | |||
There was a problem hiding this comment.
question (documentation): Clarify the reason for changing the type annotation to np.floating[Any].
The change from np.float_ to np.floating[Any] broadens the type. Please clarify if this is intentional and if there are any specific reasons for this change.
|
|
||
| A few notebooks have been included in a separate repository that can be found [here](https://github.com/leaver2000/nzthermo-notebooks). | ||
|
|
||
| ... |
There was a problem hiding this comment.
question (documentation): Clarify the purpose of the ellipsis.
The ellipsis at the end of the Notebooks section might confuse readers. Please clarify its purpose or consider removing it.
Summary by Sourcery
This pull request simplifies the constructor signatures in the
wind_componentsandwind_vectorclasses by removing parameter names. It also updates the type annotations in theparcel_profilefunction signature in the README and adds a new section to reference additional notebooks available in a separate repository.wind_componentsandwind_vectorclasses by removing parameter names.parcel_profilefunction signature in the README.