Code Review JuML

[gericke-n]

l# The Good
When profiling your function parallelplot it looks like type stability is good.
The module is easy to use and easy to understand.
Very compact code with a lot of comments explaining every step.
Good errorhandling!
The use of observables is new to me but it is nice to have this implemented. The video functionality is nice and could actually be its own feature/function instead of just a testcase!

The Suggestions

Documentation and Readme

• a documentation hosted on github would be nice to have (however since you are kind of a submodule of makie i understand it is difficult regarding the formating)!
• the readme.md includes examples which is good however it might be cleaner if you would put the comment inside of the code blocks as plain text inside the readme.md e.g.

If you want to normalize the Data, you can add the value normalized=true, default is false.
(also normalized should be normalize in the comment?)

parallelplot(DataFrame(height=160:180,weight=reverse(60:80),age=20:40),normalize=true)

-for the readme the description of color_feature might be nice? since i am not too sure what this means

Code

• maybe be careful with the @assert macro in update_plotline 183. (it could be that in future updates of julia the macro will not be maintained and disabled, however nobody is sure as to when) and documentation says it should only be used as a debugging tool (see) and (documentation @assert) (something we are currently working on as well). Maybe you could find an alternative solution as well?

• the normalize_DF function when profiling seems to be unstable. maybe this can be fixed by declaring which types in the DataFrames are possible beforehand?

• did you talk to Adrian about including his julia installation guide in the readme, since this is a public repo? (might be a bit paranoid for personal reasons)

The Nitpicking

• in ParallelPlots,jl in line 159 is a 1. Not sure if it is there for a reason.

• when running your tests the warning on line 160 appears multiple times

• Warning: too little Colors(6) are available for the Lines(7). You can set more with the 'custom_colors' attribute @ ParallelPlots C:\Users\PC\Downloads\ParallelPlots-main\ParallelPlots-main\src\ParallelPlots.jl:160

• the video you create is really cool, however it doesn't get updated properly at least for the create_person_df because you always take n_samples = 5 as an argument see: video. i am not sure if this is intended and if it is this point is moot

• ParallelPlots.jl line 141-143 intendation

-when calling create_person_df the weight is a randn which means negative weights can occur. never seen a person with negative weight tbh. (this is probably the most nitpicky one)

Additional Features

• From what i understood your labels are given by the DataFrames but can also be manually added. However if I understood Leon correctly the labels have to be added for all DF. It would maye be good to change that it is possible to only change one of the labels (however it could also be debated that you could always change the name of the labels inside the DF which should be changed)
• Video Function could be its own function and part of the package. however i am not sure if this feature is useful for anything. might be nice though for simulation of time variant data?

[gericke-n]

This is longer than i expected it to be. Sorry for that 😅

[leonhaufe]

Maybe the Person is 2147483647 Kg heavy and eats a cookie :P
fixed :D