-
Notifications
You must be signed in to change notification settings - Fork 1
Description
Hi all, I've taken a quick look at your package and have some feedback on what is "good practice" in Julia:
-
When writing a package, it's better practice to explicitly import the functions you want to use (e.g.
using Interpolations: somefunction, anotherfunction) instead of importing the whole package:
ParallelPlots/src/ParallelPlots.jl
Lines 3 to 5 in 2c29c9c
using CairoMakie using DataFrames using Interpolations -
You currently manually normalize columns in your
DataFrame:
ParallelPlots/src/ParallelPlots.jl
Lines 10 to 11 in 2c29c9c
data[!, col] = (data[!, col] .- minimum(data[!, col])) ./ (maximum(data[!, col]) - minimum(data[!, col])) Instead, you could use transformations from DataFrames.jl:
https://dataframes.juliadata.org/stable/man/basics/#Basic-Usage-of-Transformation-Functions -
Julia has an
isnothingfunction:ParallelPlots/src/ParallelPlots.jl
Line 19 in 2c29c9c
if data === nothing -
I don't think this belongs in the docstring:
ParallelPlots/src/ParallelPlots.jl
Line 37 in 2c29c9c
- Julia version: 1.10.5 -
The documentation is incomplete:
ParallelPlots/src/ParallelPlots.jl
Lines 46 to 51 in 2c29c9c
- `data::DataFrame`: - `normalize::Bool`: - `color_feature::String || nothing`: select, which axis/feature should be used for the coloring (e.g. 'weight') (default: last) - `title::String`: - `feature_labels::String`: - `curve::Bool`: -
Since you only implement one
Makie.plot!method, you probably don't need to specialize on the parametric type ofParallelPlot.function Makie.plot!(pp::ParallelPlot)should also work.
ParallelPlots/src/ParallelPlots.jl
Line 108 in 2c29c9c
function Makie.plot!(pp::ParallelPlot{<:Tuple{<:DataFrame}}) -
You might want to break down your
Makie.plot!method into small subfunctions. This is generally a good pattern in Julia and can help with compiler optimizations and performance. A good candidate for this would be the loop drawing lines from L240 to L287. -
The types in your
interpolatefunction might be too specific:
ParallelPlots/src/ParallelPlots.jl
Line 374 in 2c29c9c
function interpolate(last_x::Float64, current_x::Float64, last_y::Float64, current_y::Float64, x::Float64)
If you want to force that all arguments need to have the same type, you could writefunction interpolate(last_x::T, current_x::T, last_y::T, current_y::T, x::T) where {T<:Real}
-
It looks like your documentation is not building correctly. If you need help, don't hesitate to contact me. It might be helpful to first build the documentation locally: