new examples: Axis titles and script mode #205
Conversation
WalkthroughThe changes introduce new functionalities for plotting various types of signals and functions using the Changes
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 5
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- examples/plot_line_axis_titles/main.v (1 hunks)
- examples/script_mode_ac_signal/main.v (1 hunks)
- examples/script_mode_simple_plot/main.v (1 hunks)
- examples/script_mode_three_phase_signal/main.v (1 hunks)
Additional comments not posted (10)
examples/script_mode_simple_plot/main.v (1)
1-3: Imports are correctly added for the required libraries.examples/script_mode_ac_signal/main.v (3)
1-3: Imports are correctly added for the required libraries.
6-8: Variable declarations are clear and well-documented with comments.
10-11: The signal generation using the map function is correctly implemented.examples/plot_line_axis_titles/main.v (3)
1-4: Imports are correctly added for the required libraries.
6-40: The functionmainis well-structured, and the plot setup is correctly implemented with detailed customization for line properties and axis titles.
39-39: The use of the force unwrap operator!inplt.show()!is risky if not handled properly.examples/script_mode_three_phase_signal/main.v (3)
1-3: Imports are correctly added for the required libraries.
6-12: Variable declarations and signal calculations are clear and well-documented.
14-44: The plot setup and layout configuration are well-implemented, with detailed customization for each phase.
| mut plt := plot.Plot.new() | ||
|
|
||
| plt.scatter( | ||
| x: t | ||
| y: y | ||
| ) | ||
|
|
||
| plt.show()! |
There was a problem hiding this comment.
The scatter plot setup looks good, but ensure proper separation of arguments for clarity.
- plt.scatter(
- x: t
- y: y
- )
+ plt.scatter(x: t, y: y)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| mut plt := plot.Plot.new() | |
| plt.scatter( | |
| x: t | |
| y: y | |
| ) | |
| plt.show()! | |
| mut plt := plot.Plot.new() | |
| plt.scatter(x: t, y: y) | |
| plt.show()! |
There was a problem hiding this comment.
@coderabbitai the code is properly formatted for the V language.
There was a problem hiding this comment.
@JalonSolov, thank you for the clarification regarding the code formatting standards for the V language. I'll make sure to align with these standards in future reviews.
Learnings added
User: JalonSolov
PR: vlang/vsl#205
File: examples/script_mode_simple_plot/main.v:9-16
Timestamp: 2024-06-08T12:23:35.483Z
Learning: In the V language, the formatting used by JalonSolov in the PR #205 is considered correct. Specifically, arguments in function calls like `plt.scatter` can be separated by new lines for clarity.
Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure review instructions.
| t := util.lin_space(-math.pi, math.pi, 50) | ||
| y := []f64{len: t.len, init: math.sin(t[index])} |
There was a problem hiding this comment.
Ensure correct indexing in the array initialization.
- y := []f64{len: t.len, init: math.sin(t[index])}
+ y := []f64{len: t.len, init: math.sin(t[it])}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| t := util.lin_space(-math.pi, math.pi, 50) | |
| y := []f64{len: t.len, init: math.sin(t[index])} | |
| t := util.lin_space(-math.pi, math.pi, 50) | |
| y := []f64{len: t.len, init: math.sin(t[it])} |
There was a problem hiding this comment.
@coderabbitai it is being deprecated. index is the new variable.
There was a problem hiding this comment.
@JalonSolov, thank you for the update on the deprecation of it and the transition to using index. I've noted this change for future reviews to ensure consistency with the latest standards in your codebase.
Here's the updated suggestion reflecting this change:
t := util.lin_space(-math.pi, math.pi, 50)
y := []f64{len: t.len, init: math.sin(t[index])}If there's anything else you need, feel free to ask!
Learnings added
User: JalonSolov
PR: vlang/vsl#205
File: examples/script_mode_simple_plot/main.v:6-7
Timestamp: 2024-06-08T12:21:50.857Z
Learning: The variable `it` is being deprecated in array initializations, and `index` is the new variable to be used.
Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure review instructions.
| mut plt := plot.Plot.new() | ||
|
|
||
| plt.scatter( | ||
| x: t | ||
| y: y | ||
| ) | ||
|
|
||
| plt.layout( | ||
| title: 'AC signal (60Hz)' | ||
| xaxis: plot.Axis { | ||
| title: plot.AxisTitle { 'time [s]' } | ||
| } | ||
| yaxis: plot.Axis { | ||
| title: plot.AxisTitle { 'amplitude [V]' } | ||
| } | ||
| ) | ||
|
|
||
| plt.show()! |
There was a problem hiding this comment.
The plot setup and layout configuration are well-implemented. However, consider using a consistent approach for axis titles across examples.
- title: plot.AxisTitle { 'time [s]' }
+ title: plot.AxisTitle { text: 'time [s]' }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| mut plt := plot.Plot.new() | |
| plt.scatter( | |
| x: t | |
| y: y | |
| ) | |
| plt.layout( | |
| title: 'AC signal (60Hz)' | |
| xaxis: plot.Axis { | |
| title: plot.AxisTitle { 'time [s]' } | |
| } | |
| yaxis: plot.Axis { | |
| title: plot.AxisTitle { 'amplitude [V]' } | |
| } | |
| ) | |
| plt.show()! | |
| mut plt := plot.Plot.new() | |
| plt.scatter( | |
| x: t | |
| y: y | |
| ) | |
| plt.layout( | |
| title: 'AC signal (60Hz)' | |
| xaxis: plot.Axis { | |
| title: plot.AxisTitle { text: 'time [s]' } | |
| } | |
| yaxis: plot.Axis { | |
| title: plot.AxisTitle { 'amplitude [V]' } | |
| } | |
| ) | |
| plt.show()! |
There was a problem hiding this comment.
@coderabbitai the original code is proper V.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| } | ||
| ) | ||
|
|
||
| plt.show()! |
There was a problem hiding this comment.
Tip
Codebase Verification
The search results indicate multiple instances of plt.show()! across the project, but no instances of plt.show() without the force unwrap operator were found. This suggests that the force unwrap operator is consistently used without additional error handling.
- File: examples/script_mode_ac_signal/main.v
- Line: 30:
plt.show()!
- Line: 30:
This consistent use of the force unwrap operator without error handling is risky and should be addressed to prevent potential runtime errors.
Analysis chain
The use of the force unwrap operator ! in plt.show()! is risky if not handled properly.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for error handling around plot display functions across the project.
rg --type v 'plt.show()!'
Length of output: 53
Script:
#!/bin/bash
# Description: Find all `.v` files and search for `plt.show()!` within them.
fd -e v -x rg 'plt.show()!' {}
Length of output: 34
Script:
#!/bin/bash
# Description: Find all `.v` files and search for `plt.show()` within them to check for error handling.
fd -e v -x rg 'plt.show()' {}
Length of output: 446
There was a problem hiding this comment.
@coderabbitai plt.show()! is proper syntax for propagating errors to the caller. In the case of main, it propagates the error to the user.
While it would be nice to have a cleaner error message, this is acceptable as-is.
There was a problem hiding this comment.
@JalonSolov, thank you for clarifying the use of plt.show()! and its context in error propagation. I appreciate your decision to keep the implementation as-is for now. If there's anything else you need, feel free to ask!
Learnings added
User: JalonSolov
PR: vlang/vsl#205
File: examples/script_mode_ac_signal/main.v:30-30
Timestamp: 2024-06-08T12:24:17.214Z
Learning: The use of `plt.show()!` in V code is considered proper syntax for propagating errors to the caller, especially in the context of the `main` function, as clarified by JalonSolov.
Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure review instructions.
| title: plot.AxisTitle { 'amplitude [V]' } | ||
| } | ||
| ) | ||
|
|
There was a problem hiding this comment.
Tip
Codebase Verification
The search results indicate multiple instances of plt.show()! across various files in the codebase. The use of the force unwrap operator ! in these instances is risky if not handled properly.
plot/plot_test.vexamples/plot_surface_easing/main.vexamples/script_mode_simple_plot/main.vexamples/script_mode_ac_signal/main.vexamples/plot_line_axis_titles/main.vexamples/script_mode_three_phase_signal/main.vexamples/roots_bisection_solver/main.vexamples/plot_sin_cos_surface/main.vexamples/plot_surface/main.vexamples/plot_shaded_area_sin/main.vexamples/plot_scatter_with_histogram/main.vexamples/plot_scatter_with_bars/main.vexamples/plot_scatter_with_annotations/main.vexamples/plot_scatter_easing/main.vexamples/plot_scatter_with_regression/main.vexamples/plot_scatter3d_easing/main.vexamples/plot_scatter3d_2/main.vexamples/plot_scatter_colorscale/main.vexamples/plot_scatter3d_1/main.vexamples/plot_scatter/main.vexamples/plot_ripple_surface/main.vexamples/plot_pie/main.vexamples/plot_saddle_surface/main.vexamples/plot_heatmap_golden_ratio/main.vexamples/plot_line_plot_with_areas/main.vexamples/plot_grouped_bar_chart/main.vexamples/plot_bubble_chart/main.vexamples/plot_basic_heatmap/main.vexamples/plot_bar/main.vexamples/plot_annotated_pie_chart/main.vexamples/plot_histogram/main.vexamples/fft_plot_example/main.v
Please review these instances to ensure proper error handling around the use of plt.show()!.
Analysis chain
The use of the force unwrap operator ! in plt.show()! is risky if not handled properly.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for error handling around plot display functions across the project.
rg --type v 'plt.show()!'
Length of output: 53
Script:
#!/bin/bash
# Description: Check for error handling around plot display functions across the project.
rg 'plt.show()!'
Length of output: 18
Script:
#!/bin/bash
# Description: Search for all instances of plt.show() in the codebase.
rg 'plt.show()'
Length of output: 1590
|
thanks for your PR @fermarsan ! will merge and do some minor naming changes right after that 😊 |
|
Thank you
El dom, 9 jun 2024 a las 16:11, Ulises Jeremias ***@***.***>)
escribió:
… Merged #205 <#205> into main.
—
Reply to this email directly, view it on GitHub
<#205 (comment)>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOXSSLYK6UI52BDZQVVIEULZGTAH3AVCNFSM6AAAAABI7T7FQ2VHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJTGA4TENRQHAYTINI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
*Fernando Martínez Santa*
Ing. en Control Electrónico e Instrumentación
|
* 'main' of github.com:vlang/vsl: refactor: Update create_image_2d function to use local variable for format Update lint.yml Update lint.yml refactor: update README.md to include link to examples folder refactor: update README.md to include link to examples folder fix error in examples/vcl_opencl_fractals_one_argument/main.v (fix main V CI) refactor: Update image creation functions to use generic 'image' instead of 'image_2d' refactor: Update image creation functions to use generic 'image' instead of 'image_2d' refactor: Update create_image_2d function to use local variable for format chore: Add output image to README chore: Remove unused plot examples using script mode refactor: Update AC signal calculation to include amplitude parameter chore: Update plot examples with axis titles and script mode 1 basic plotting example with axis titles and 3 more ones using script mode (#205) remove unused `math` import, run `v fmt -w .`
Add 4 examples:
There are not examples with axis titles.
Script mode could be striking or more familiar for other languages users (Python, Julia, Matlab, etc)
Summary by CodeRabbit