Started working updating vsl.plot #181
Conversation
WalkthroughThe changes primarily focus on simplifying the code and enhancing the functionality of the plot module. The modifications include the removal of unnecessary mapping operations, the addition of new structs for plot configuration, and the enhancement of existing structs to improve plot layout and annotation. Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Files ignored due to filter (1)
- plot/static/plotly-2.26.2.min.js
Files selected for processing (9)
- examples/plot_heatmap_golden_ratio/main.v (1 hunks)
- examples/plot_scatter/main.v (1 hunks)
- examples/plot_scatter3d_1/main.v (1 hunks)
- examples/plot_scatter_colorscale/main.v (1 hunks)
- examples/plot_scatter_with_annotations/main.v (1 hunks)
- plot/annotation.v (1 hunks)
- plot/layout.v (1 hunks)
- plot/show.v (1 hunks)
- plot/trace.v (1 hunks)
Files skipped from review due to trivial changes (3)
- examples/plot_scatter/main.v
- examples/plot_scatter_colorscale/main.v
- plot/trace.v
Additional comments (Suppressed): 14
examples/plot_scatter_with_annotations/main.v (1)
- 6-43: The changes in this hunk look good. The code is now encapsulated within a
mainfunction, which is a good practice for readability and maintainability. Thexrange is now directly assigned usingutil.arange(y.len), which simplifies the code. Thescatterfunction call and thelayoutfunction call have been updated to match the new struct definitions. Theshowfunction call is now properly error-handled with the!operator.examples/plot_scatter3d_1/main.v (1)
- 22-22: The new hunk has removed the
.map(f64(it))operation onx. This operation was converting the range of integers to a range of floats. If thescatter3dfunction or any other function that usesxexpects a float array, this could cause type mismatch errors. Please verify if this change is intentional and compatible with the rest of the codebase.- x := util.arange(y.len) + x := util.arange(y.len).map(f64(it))plot/annotation.v (3)
6-7: The
xandyfields are now optional with theomitemptyattribute. Ensure that the rest of the codebase can handle these fields being potentially empty, and that it doesn't lead to any runtime errors or unexpected behavior.8-8: The
textfield is now required. Make sure that all instances ofAnnotationin the codebase are updated to include this field to avoid any runtime errors.9-9: The
showarrowfield is no longer optional. If there are instances in the codebase where this field was not provided, they will need to be updated.plot/layout.v (2)
7-8: The
title_xandautosizefields no longer have default values. This could potentially break existing code that relies on these defaults. Please ensure that all instances whereLayoutis used have been updated to explicitly set these fields if needed.9-9: The
widthfield now defaults to 550. This is a change from the previous version where no default was set. This change should not cause any issues, but it's worth verifying that this new default value is appropriate in all contexts whereLayoutis used.examples/plot_heatmap_golden_ratio/main.v (2)
7-40: The new hunk has encapsulated the existing code into a
mainfunction, which is a good practice for structuring the code. However, the commented lines for drawing a spiral are still present and marked as a TODO. If this functionality is not intended to be implemented in the near future, consider removing these comments to avoid confusion.26-39: The
heatmapandlayoutmethods are called on thepltinstance to create a heatmap plot and set its properties, respectively. Theshowmethod is then called to display the plot. Ensure that these methods are compatible with the changes made to thevsl.plotmodule in this PR.plot/show.v (5)
16-37: The
showfunction now accepts aPlotConfigparameter, which is used to configure the plot. Ensure that all calls to this function throughout the codebase have been updated to pass this parameter.49-93: The
get_plotly_scriptfunction generates the plot script as an HTML tag. This is a good practice as it separates the generation of the plot script from the rest of the HTML content, improving modularity and maintainability of the code.95-110: The
get_htmlfunction now uses theget_plotly_scriptfunction to generate the plot script. This is a good practice as it separates the generation of the plot script from the rest of the HTML content, improving modularity and maintainability of the code.119-130: The
handlefunction now uses theget_htmlfunction to generate the HTML content for the plot. This is a good practice as it separates the generation of the HTML content from the HTTP handling logic, improving modularity and maintainability of the code.134-150: The
encodefunction has been updated to also replace occurrences of"[]int"and"[][]f64". This is a good practice as it ensures that all types of arrays are correctly encoded into JSON.
| use_cdn bool | ||
| } |
There was a problem hiding this comment.
The PlotConfig struct only contains a single field use_cdn. If this is the only configuration option for the plot, it might be more straightforward to pass this as a boolean parameter to the show function and get_html function, rather than wrapping it in a struct. This would simplify the function signatures and make the code easier to understand.
| use_cdn: true | ||
| plot: p |
There was a problem hiding this comment.
The use_cdn field of the PlotlyHandler struct is always set to true, ignoring the value in the PlotConfig parameter. This should be changed to use the value from the PlotConfig parameter.
- use_cdn: true
+ use_cdn: config.use_cdn| // PlotlyScriptConfig is a configuration for the Plotly plot script. | ||
| [params] | ||
| pub struct PlotlyScriptConfig { | ||
| PlotConfig | ||
| } |
There was a problem hiding this comment.
The PlotlyScriptConfig struct is defined but it only inherits from PlotConfig and doesn't add any new fields. If there are no additional configuration options for the plot script, it might be unnecessary to define a separate struct for it. Consider using PlotConfig directly instead.
Summary by CodeRabbit
plot_scatter,plot_scatter3d_1, andplot_scatter_colorscaleexamples by removing unnecessary mapping operations, improving code readability.PlotConfigandPlotlyScriptConfiginplot/show.vto provide more configuration options for Plotly plots, enhancing customization.get_plotly_scriptfunction inplot/show.vto generate plot scripts as HTML tags, enabling easier integration with web pages.Layoutstruct inplot/layout.vto improve plot layout configuration, enhancing the visual appeal of plots.Annotationstruct inplot/annotation.vto require certain fields, ensuring more consistent annotations.XType,YType, andZTypeinplot/trace.vto accept arrays of integers, broadening data type compatibility.