💨wind vector stuff #19
Conversation
Reviewer's Guide by SourceryThis pull request refactors wind-related functions by introducing new classes for wind_vector and wind_components, along with conversion constructors. It also updates ufunc implementations and signatures, adds new thermodynamic constants, and improves documentation and test coverage. 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: 8 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue 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.
|
|
||
| return wdir; | ||
| constexpr T wind_direction(const T u, const T v) noexcept { | ||
| return fmod(degrees(atan2(u, v)) + 180.0, 360.0); |
There was a problem hiding this comment.
issue (bug_risk): Potential issue with wind direction calculation.
The new implementation of wind_direction always adds 180 degrees, which might not be the intended behavior. The previous implementation had a conditional check for the from parameter. Please verify if this change is intentional.
| template <floating T> | ||
| constexpr T wind_direction(const wind_components<T>& uv) noexcept { |
There was a problem hiding this comment.
suggestion: Redundant function definition.
The new wind_direction function that takes wind_components<T> as a parameter seems redundant since it just calls the existing wind_direction function. Consider removing it to avoid unnecessary code duplication.
| template <floating T> | |
| constexpr T wind_direction(const wind_components<T>& uv) noexcept { | |
| template <floating T> | |
| constexpr T wind_direction(const wind_vector<T>& uv) noexcept { | |
| return wind_direction(wind_components<T>(uv)); | |
| } |
| T speed; | ||
| T direction; | ||
| }; | ||
| class wind_vector; |
There was a problem hiding this comment.
suggestion: Forward declaration of wind_vector.
The forward declaration of wind_vector is not necessary here since the full definition is provided later in the file. Consider removing the forward declaration to improve code clarity.
| class wind_vector; | |
| template <floating T> |
| @cython.ufunc | ||
| cdef (double, double) wind_vector(T u, T v) noexcept nogil: |
There was a problem hiding this comment.
suggestion: Inconsistent function naming.
The function wind_vector might be better named as wind_vector_from_components to clearly indicate its purpose and avoid confusion with the wind_vector class.
| @cython.ufunc | |
| cdef (double, double) wind_vector(T u, T v) noexcept nogil: | |
| @cython.ufunc | |
| cdef (double, double) wind_vector_from_components(T u, T v) noexcept nogil: |
| T0 = C.T0 | ||
| """`(J/kg*K)` - freezing point in kelvin""" | ||
| E0 = C.E0 | ||
| """`(Pa)` - vapor pressure at T0""" | ||
| Cp = C.Cp | ||
| """`(J/kg*K)` - specific heat of dry air""" | ||
| Rd = C.Rd | ||
| """`(J/kg*K)` - gas constant for dry air""" | ||
| Rv = C.Rv | ||
| """`(J/kg*K)` - gas constant for water vapor""" |
There was a problem hiding this comment.
suggestion: Missing documentation for constants.
Consider adding documentation comments for the newly added constants (T0, E0, Cp, etc.) to improve code readability and maintainability.
| @@ -66,12 +66,12 @@ constexpr T potential_temperature(const T pressure, const T temperature) noexcep | |||
| template <floating T> | |||
| constexpr T equivalent_potential_temperature( | |||
| const T pressure, const T temperature, const T dewpoint | |||
| ) noexcept; // theta_e | |||
There was a problem hiding this comment.
suggestion: Removed comment for function purpose.
The comment // theta_e was removed. Consider re-adding it or providing a more detailed comment to describe the purpose of the equivalent_potential_temperature function.
| @@ -66,12 +66,12 @@ | |||
| template <floating T> | |||
| constexpr T equivalent_potential_temperature( | |||
| const T pressure, const T temperature, const T dewpoint | |||
| ) noexcept; // theta_e | |||
There was a problem hiding this comment.
suggestion: Removed comment for function purpose.
The comment // theta_w was removed. Consider re-adding it or providing a more detailed comment to describe the purpose of the wet_bulb_potential_temperature function.
| ) noexcept; // theta_e | |
| template <floating T> | |
| constexpr T wet_bulb_potential_temperature( | |
| const T pressure, const T temperature, const T dewpoint | |
| ) noexcept; // Calculates the wet-bulb potential temperature (theta_w) |
| // copy constructor | ||
| constexpr lcl(const lcl<T>& other) noexcept = default; | ||
| // move constructor | ||
| constexpr lcl(lcl<T>&& other) noexcept = default; | ||
| // copy assignment operator | ||
| constexpr lcl<T>& operator=(const lcl<T>& other) noexcept = default; | ||
| // destructor | ||
| ~lcl() noexcept = default; |
There was a problem hiding this comment.
question: Removed special member functions.
The copy constructor, move constructor, copy assignment operator, and destructor for the lcl class were removed. If these were intentionally removed, consider documenting why they are no longer needed.
| u, v = wind_components(WIND_DIRECTIONS, WIND_MAGNITUDES) | ||
| assert_allclose([WIND_DIRECTIONS, WIND_MAGNITUDES], wind_vector(u, v), atol=1e-9) |
There was a problem hiding this comment.
suggestion (testing): Test for edge cases with zero wind magnitude
Consider adding tests for edge cases where the wind magnitude is zero. This will ensure that the wind_vector function handles these cases correctly.
| u, v = wind_components(WIND_DIRECTIONS, WIND_MAGNITUDES) | |
| assert_allclose([WIND_DIRECTIONS, WIND_MAGNITUDES], wind_vector(u, v), atol=1e-9) | |
| u, v = wind_components(WIND_DIRECTIONS, WIND_MAGNITUDES) | |
| assert_allclose([WIND_DIRECTIONS, WIND_MAGNITUDES], wind_vector(u, v), atol=1e-9) | |
| zero_magnitude = [0, 0, 0] | |
| zero_directions = [0, 90, 180] | |
| u_zero, v_zero = wind_components(zero_directions, zero_magnitude) | |
| assert_allclose([zero_directions, zero_magnitude], wind_vector(u_zero, v_zero), atol=1e-9) |
Summary by Sourcery
This pull request introduces new classes
wind_vectorandwind_componentsto encapsulate wind data, refactors existing wind-related functions to use these classes, and adds corresponding tests and documentation.wind_vectorandwind_componentsclasses to encapsulate wind data and provide conversion methods between direction/magnitude and u/v components.wind_vectorandwind_componentsclasses.wind_directionfunction to simplify its logic and remove thefromparameter.wind_componentsandwind_vectorclasses with explicit constructors for better initialization and conversion.theta_eandtheta_wfunctions inlibthermo.cpp.T0,E0,Cp,Rd,Rv,Lv,P0,Mw,Md,epsilon, andkappainsrc/nzthermo/_core.pyx.wind_vectorfunction to ensure correct conversion between wind components and direction/magnitude.