poly: improvements, tests and examples #208
Conversation
WalkthroughThe recent updates refine polynomial evaluation and manipulation in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Main
participant Poly as vsl.poly
User ->> Main: Run poly_operations example
Main ->> Poly: Call add, subtract, multiply, divide functions
Poly -->> Main: Return results
Main -->> User: Display results
Possibly related PRs
Poem
Tip OpenAI O1 model for chat
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (3)
Files skipped from review as they are similar to previous changes (1)
Additional comments not posted (11)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- poly/poly.v (2 hunks)
- poly/poly_test.v (3 hunks)
Additional comments not posted (4)
poly/poly_test.v (2)
5-9: Ensure test_eval reflects the updated eval logic.The comments and assertions in
test_evalseem to align with the updatedevalfunction logic. The test case appears to correctly validate the expected outcome based on the new loop structure and initialization ofansto 0.0.
39-44: Review the updated test_multiply for correctness.The
test_multiplyfunction has been updated with new calculations and assertions. Ensure that the expected results in the assertion match the expected polynomial multiplication output. This test case should be verified to ensure it accurately reflects the changes in themultiplyfunction.poly/poly.v (2)
13-17: Approved changes in eval function.The updated
evalfunction initializesansto 0.0 and correctly iterates through the coefficients in reverse order, ensuring that each coefficient is correctly factored into the result. This change aligns with the performance improvements noted in the PR description.
136-142: Verify the logic of sorted_3_ function.The
sorted_3_function has been updated to ensure correct sorting by adjusting the conditional checks and variable assignments. It's crucial to verify that this function now correctly sorts any three values.Verification successful
Verify the logic of
sorted_3_function.The
sorted_3_function is implemented inpoly/poly.vand is tested inpoly/poly_test.v. The test case inpoly/poly_test.vverifies the sorting logic. Please ensure that the test case comprehensively covers different scenarios to confirm the correctness of the function.
Location of
sorted_3_function implementation:
poly/poly.v: fn sorted_3_(x_ f64, y_ f64, z_ f64) (f64, f64, f64) {Location of test case:
poly/poly_test.v: fn test_sorted_3_() {Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the logic of the sorted_3_ function by checking its usage and ensuring it sorts correctly. # Test: Search for the sorted_3_ function usage. Expect: Only occurances of the new signature. rg --type v -A 5 $'sorted_3_'Length of output: 54
Script:
#!/bin/bash # Description: Verify the logic of the sorted_3_ function by checking its usage and ensuring it sorts correctly. # Test: Search for the sorted_3_ function usage in all .v files. rg 'sorted_3_' --glob '*.v' -A 5Length of output: 820
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- poly/poly.v (2 hunks)
- poly/poly_test.v (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- poly/poly.v
- poly/poly_test.v
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
examples/poly_eval/README.md (1)
3-3: Add missing article for grammatical correctness.Consider adding "a" before "polynomial" to improve the grammatical correctness of the sentence.
- This example demonstrates the usage of the V Scientific Library for evaluating polynomial at given value of x. + This example demonstrates the usage of the V Scientific Library for evaluating a polynomial at a given value of x.Tools
LanguageTool
[uncategorized] ~3-~3: You might be missing the article “a” here.
Context: ...ic Library for evaluating polynomial at given value of x. ## Instructions 1. Ensure...(AI_EN_LECTOR_MISSING_DETERMINER_A)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- examples/poly_eval/README.md (1 hunks)
- examples/poly_eval/main.v (1 hunks)
- examples/poly_operations/README.md (1 hunks)
- examples/poly_operations/main.v (1 hunks)
Files skipped from review due to trivial changes (2)
- examples/poly_eval/main.v
- examples/poly_operations/README.md
Additional context used
LanguageTool
examples/poly_eval/README.md
[uncategorized] ~3-~3: You might be missing the article “a” here.
Context: ...ic Library for evaluating polynomial at given value of x. ## Instructions 1. Ensure...(AI_EN_LECTOR_MISSING_DETERMINER_A)
Additional comments not posted (2)
examples/poly_eval/README.md (1)
1-17: Review of README content.The README provides clear instructions on how to run the polynomial evaluation example using the V Scientific Library. The steps are well-documented and include links to necessary resources.
Tools
LanguageTool
[uncategorized] ~3-~3: You might be missing the article “a” here.
Context: ...ic Library for evaluating polynomial at given value of x. ## Instructions 1. Ensure...(AI_EN_LECTOR_MISSING_DETERMINER_A)
examples/poly_operations/main.v (1)
1-23: Review of polynomial operations example.The example demonstrates polynomial operations using the VSL effectively. The code is clean and well-commented, providing clear insights into the operations being performed.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- examples/poly_operations/main.v (1 hunks)
- poly/poly.v (3 hunks)
- poly/poly_test.v (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- examples/poly_operations/main.v
- poly/poly_test.v
Additional comments not posted (2)
poly/poly.v (2)
13-17: Refactoredevalfunction: Correctness and PerformanceThe refactoring of the
evalfunction from a forward iteration to a reverse iteration approach is a significant improvement. This change not only corrects the previously incorrect results but also enhances performance, as demonstrated in the PR description. The use of a reverse iteration aligns well with the Horner's method for polynomial evaluation, which is known for its efficiency.
136-142: Refactoredsorted_3_function: Logic and EfficiencyThe changes to the
sorted_3_function's logic, where the sequence of comparisons and swaps has been adjusted, are critical for ensuring the correct sorting of the three values. The new sequence of swaps ensures that the smallest value is correctly placed first, followed by the second smallest, and then the largest, which is a more logical and efficient approach than the previous one.However, it's important to verify that this new logic indeed sorts the values correctly in all scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- examples/poly_operations/main.v (1 hunks)
- poly/poly.v (4 hunks)
- poly/poly_test.v (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- examples/poly_operations/main.v
- poly/poly_test.v
Additional comments not posted (2)
poly/poly.v (2)
13-17: Approved changes inevalfunction.The changes optimize polynomial evaluation using a decrementing loop, which is a typical implementation of Horner's method for evaluating polynomials efficiently. The transformation is correctly implemented.
However, ensure that the function handles cases where the coefficients array might be empty, as indicated by the initial panic check.
Verification successful
Approved changes in
evalfunction.The changes optimize polynomial evaluation using a decrementing loop, which is a typical implementation of Horner's method for evaluating polynomials efficiently. The transformation is correctly implemented.
Additionally, the function includes a panic condition to handle cases where the coefficients array might be empty, ensuring robust error handling.
- Verified the handling of empty coefficients with the panic condition at the start of the function.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify handling of empty coefficients in `eval` function. # Test: Search for the panic condition when coefficients are empty. Expect: Proper handling of empty input. rg --type v -A 5 $'if c.len == 0'Length of output: 60
Script:
#!/bin/bash # Description: Verify handling of empty coefficients in `eval` function. # Test: Search for the panic condition when coefficients are empty. Expect: Proper handling of empty input. rg -A 5 'if c.len == 0'Length of output: 205
136-142: Approved changes insorted_3_function.The updated logic in
sorted_3_function ensures correct sorting of three values using a simple and effective conditional structure, akin to a bubble sort for three elements. The changes are correctly implemented to ensure that values are sorted in ascending order.However, verify that the sorting logic always results in the correct order by comparing against a variety of test cases.
|
I have a problem with the |
PottierLoic
changed the title
PottierLoic
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
examples/poly_eval/main.v (1)
5-10: Ensure consistent types in the coefficients list.The coefficients list should use consistent types for better readability and to avoid potential issues.
- coef := [4.0, 5, 2] + coef := [4.0, 5.0, 2.0]
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- examples/poly_eval/main.v (1 hunks)
- poly/poly_test.v (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- poly/poly_test.v
Additional comments not posted (2)
examples/poly_eval/main.v (2)
1-4: LGTM!The module declaration and import statement are correct.
12-16: LGTM!The example correctly evaluates the polynomial derivatives.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- examples/poly_matrix/README.md (1 hunks)
- examples/poly_matrix/main.v (1 hunks)
- poly/poly_test.v (2 hunks)
Files skipped from review due to trivial changes (1)
- examples/poly_matrix/README.md
Additional comments not posted (14)
examples/poly_matrix/main.v (4)
1-4: LGTM!The module declaration and import statement are correct and necessary.
5-8: LGTM!The main function and polynomial coefficients are correctly defined.
9-19: LGTM!The companion matrix calculation and printing are correct and follow the mathematical definition.
21-26: LGTM!The balancing matrix calculation and printing are correct and follow the mathematical definition.
poly/poly_test.v (10)
5-10: LGTM!The test case for the
evalfunction is correct and verifies the expected output.
12-17: LGTM!The test case for the
eval_derivsfunction is correct and verifies the expected output.
19-24: LGTM!The test case for the
solve_quadraticfunction is correct and verifies the expected output.
Line range hint
26-31:
LGTM!The test case for the
swap_function is correct and verifies the expected output.
34-40: LGTM!The test case for the
sorted_3_function is correct and verifies the expected output.
42-49: LGTM!The test case for the
companion_matrixfunction is correct and verifies the expected output.
51-58: LGTM!The test case for the
balance_companion_matrixfunction is correct and verifies the expected output.
61-64: LGTM!The test case for the
addfunction is correct and verifies the expected output.
67-70: LGTM!The test case for the
subtractfunction is correct and verifies the expected output.
73-76: LGTM!The test case for the
multiplyfunction is correct and verifies the expected output.
|
Sorry for the |
I read something about normalized cubic polynomials where 𝑎 is equal to 1. Is that what the function is supposed to take as parameters? @ulises-jeremias can you explain me the goal behind this ? |
Merging with @suleyman-kaya changes removed println and useless math import in poly_test.v
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
examples/poly_operations/main.v (1)
5-33: Great examples of polynomial operations!The
mainfunction provides clear examples of using the polynomial operations from the V Scientific Library, including addition, subtraction, multiplication, and division. This is a valuable addition to the codebase.A few suggestions for improvement:
- Address the TODO comment regarding fixing the division operation. Ensure that the division example produces the expected results.
- Consider enhancing the print statements to provide more context about each operation. For example:
This will make the output more informative and easier to understand.println('Addition of ${poly_1} and ${poly_2}:') println('Result: ${result_add}')Overall, great work on demonstrating the usage of polynomial operations!
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- examples/poly_operations/main.v (1 hunks)
- poly/poly.v (2 hunks)
- poly/poly_test.v (4 hunks)
Additional comments not posted (9)
poly/poly_test.v (7)
5-7: Verify the updated expected result.The expected result for the polynomial evaluation has been updated from
53to56. It's crucial to ensure that this change aligns with the modifications made to theevalfunction's implementation.Please double-check the coefficients and the evaluation logic to confirm that the new expected result of
56is indeed correct. If necessary, provide additional test cases to cover different scenarios and boundary conditions.
9-14: LGTM!The addition of the
test_eval_derivsfunction is a valuable contribution to the test suite. It verifies the correctness of theeval_derivsfunction, which evaluates the derivatives of a polynomial at a given point.The test case is well-structured and covers the expected behavior. Great job on enhancing the test coverage for polynomial operations!
16-21: LGTM!The addition of the
test_solve_quadraticfunction is a great contribution to the test suite. It verifies the correctness of thesolve_quadraticfunction, which finds the roots of a quadratic equation.The test case is well-structured and covers the expected behavior. It's great to see the test coverage expanding to include different types of polynomial equations!
31-37: LGTM!The updated
test_sorted_3_function now includes assertions to verify the expected order of the sorted values. The assertions cover all three sorted positions, ensuring that the values are indeed sorted in ascending order.The test case is well-structured and provides comprehensive coverage of the
sorted_3_function's behavior. Great job on enhancing the test suite!
39-46: LGTM!The addition of the
test_companion_matrixfunction is a valuable contribution to the test suite. It verifies the correctness of thecompanion_matrixfunction, which constructs a companion matrix from polynomial coefficients.The test case is well-structured and covers the expected behavior. It's great to see the test coverage expanding to include matrix-related operations!
48-56: LGTM!The addition of the
test_balance_companion_matrixfunction is a great contribution to the test suite. It verifies the correctness of thebalance_companion_matrixfunction, which balances a companion matrix to improve numerical stability.The test case is well-structured and covers the expected behavior. It's excellent to see the test coverage expanding to include matrix balancing operations!
Line range hint
58-91: LGTM!The removal of print statements from the
test_add,test_subtract,test_multiply, andtest_dividefunctions is a positive change. It helps to keep the test output clean and focused on the assertions.By relying solely on assertions to verify the expected behavior, the test functions become more maintainable and less prone to errors caused by manual inspection of printed output.
Great job on improving the test suite's clarity and reliability!
poly/poly.v (2)
18-22: Verify the updated polynomial evaluation logic.The changes made to the
evalfunction modify the way polynomial evaluation is performed. The new implementation initializesansto zero and uses a decrementing indexito traverse the coefficients in reverse.While the updated logic appears to be correct, it's important to verify the following:
- Ensure that the changes do not introduce any unintended side effects or bugs.
- Confirm that the updated implementation produces the same results as the previous version for various test cases.
- Assess the performance impact of the changes, if any, to ensure that the evaluation remains efficient.
Please provide additional test cases and benchmarks to validate the correctness and performance of the updated
evalfunction.
149-156: Verify the updated sorting logic.The changes made to the
sorted_3_function modify the order in which the variables are compared and swapped during the sorting process. The updated conditions ensure that the variables are sorted in ascending order.While the new implementation appears to be correct, it's important to verify the following:
- Ensure that the changes do not introduce any unintended side effects or bugs.
- Confirm that the updated implementation produces the expected sorted order for various test cases, including edge cases and different input combinations.
Please provide additional test cases to validate the correctness of the updated
sorted_3_function and ensure that it handles all possible scenarios correctly.
|
@suleyman-kaya poly_dividend := [2.0, -4.0, -4.0, 1.0] // 2 - 4x - 4x^2 + x^3
poly_divisor := [-2.0, 1.0] // -2 + x
quotient, remainder := poly.divide(poly_dividend, poly_divisor)should be : When I reverse the order of the parameters, I get the correct result. ...
// Input: a = [a_0, ..., a_n], b = [b_0, ..., b_m]
...
pub fn divide(a []f64, b []f64) ([]f64, []f64) {Alternatively, we could modify the function so that it accepts the parameters in the order specified in the comment. |
There was a problem hiding this comment.
Thank you for efforts, all seems good! I regret that I am unable to make the necessary changes in the near future, and therefore, I must kindly request your assistance in this matter. Please feel free to employ any method you deem appropriate to resolve this issue.
|
No problem, I'll take a look when I have some time. You couldn't catch it with test_poly because the test is done with a symmetric polynomial: a := [1.0, 2.0, 1.0]
b := [1.0, 1.0]Reversing the two lists wouldn't change anything, which is why it passed :) |
|
@ulises-jeremias could you take a look please ? Also, a |
|
|
Oh ok I guess i can fmt everything in that case ? |
This PR fix eval and sorted_3_ function, they returned wrong results.
The new eval function is now faster and return the correct result:
I'll add tests for the others functions soon and I can fill some examples to resolve this : #106
Summary by CodeRabbit
New Features
Bug Fixes
Improvements