Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add errorline.jl #513

Merged
merged 25 commits into from
Aug 26, 2022
Merged

add errorline.jl #513

merged 25 commits into from
Aug 26, 2022

Conversation

CMGreenspon
Copy link
Contributor

Function for parsing inputs to easily make a ribbons (https://ggplot2.tidyverse.org/reference/geom_ribbon.html) or errorbar https://www.mathworks.com/help/matlab/ref/errorbar.html) plot while allowing for easily controlling error type and also handles NaNs.

Copy link
Member

@BeastyBlacksmith BeastyBlacksmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution. I got some stylistic and functional remarks.
Additionally we should think about whether this recipe needs to live here or could also be added to Plots.jl itself.
Currently, I don't see a reason why not.

.vscode/settings.json Outdated Show resolved Hide resolved
src/errorline.jl Outdated Show resolved Hide resolved
src/errorline.jl Outdated Show resolved Hide resolved
src/errorline.jl Outdated Show resolved Hide resolved
src/errorline.jl Outdated Show resolved Hide resolved
CMGreenspon and others added 5 commits August 23, 2022 05:28
Co-authored-by: Simon Christ <SimonChrist@gmx.de>
Co-authored-by: Simon Christ <SimonChrist@gmx.de>
Co-authored-by: Simon Christ <SimonChrist@gmx.de>
@CMGreenspon
Copy link
Contributor Author

CMGreenspon commented Aug 23, 2022

I believe I've addressed all the changes. The error you got was because NaNMath only accepts Floats so I've added a conversion check. I also added the final distribution mode.

I added a new runtest too with Ints as the y values for future checks

Copy link
Member

@BeastyBlacksmith BeastyBlacksmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far this looks fine to me, there should be at least one example in the README as Documentation. Would be good to have another pair of eyes here.

@CMGreenspon
Copy link
Contributor Author

CMGreenspon commented Aug 23, 2022 via email

Copy link
Member

@t-bltg t-bltg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a StatsPlots user so I'm only going to add stylistic comments here. Otherwise, I don't see why it cannot go in, since this PR is tested and documented.

src/errorline.jl Outdated Show resolved Hide resolved
src/errorline.jl Outdated Show resolved Hide resolved
src/errorline.jl Outdated
Comment on lines 60 to 64
if centertype == :mean
y_central = mapslices(NaNMath.mean, y, dims=2)
elseif centertype == :median
y_central = mapslices(NaNMath.median, y, dims=2)
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if centertype == :mean
y_central = mapslices(NaNMath.mean, y, dims=2)
elseif centertype == :median
y_central = mapslices(NaNMath.median, y, dims=2)
end
y_central = if centertype == :mean
mapslices(NaNMath.mean, y, dims=2)
elseif centertype == :median
mapslices(NaNMath.median, y, dims=2)
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love that line 60 is a single line, I think it makes it awkward to read, but if broken into two lines I think that's pretty good so I'll do that unless that's against convention?

src/errorline.jl Outdated Show resolved Hide resolved
src/errorline.jl Outdated Show resolved Hide resolved
src/errorline.jl Outdated Show resolved Hide resolved
src/errorline.jl Outdated Show resolved Hide resolved
src/errorline.jl Outdated Show resolved Hide resolved
num_obs = size(y,2)
if num_obs > numsecondarylines
sub_sample_idx = sample(1:num_obs, numsecondarylines, replace=false)
y_sub_sample = y[:,sub_sample_idx,g]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what convention is used in StatsPlots (maybe we need an autoformatter here as well), but I find y[:, sub_sample_idx, g] more readable.

CMGreenspon and others added 7 commits August 23, 2022 09:27
Co-authored-by: t-bltg <tf.bltg@gmail.com>
Co-authored-by: t-bltg <tf.bltg@gmail.com>
Co-authored-by: t-bltg <tf.bltg@gmail.com>
Co-authored-by: t-bltg <tf.bltg@gmail.com>
Co-authored-by: t-bltg <tf.bltg@gmail.com>
Co-authored-by: t-bltg <tf.bltg@gmail.com>
@CMGreenspon
Copy link
Contributor Author

plot_36
Example plot

@CMGreenspon
Copy link
Contributor Author

ExamplePlot
Rename spaghetti to plume

@CMGreenspon
Copy link
Contributor Author

ExamplePlot

@BeastyBlacksmith BeastyBlacksmith merged commit 8229873 into JuliaPlots:master Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants