Skip to content

Add plot recipe for pks#18

Merged
halleysfifthinc merged 14 commits intohalleysfifthinc:masterfrom
MrHenning:plot_recipe
Oct 19, 2022
Merged

Add plot recipe for pks#18
halleysfifthinc merged 14 commits intohalleysfifthinc:masterfrom
MrHenning:plot_recipe

Conversation

@MrHenning
Copy link
Contributor

@MrHenning MrHenning commented Sep 19, 2022

Hi,
I don't know if this is desirable, but I added a plot-recipe for peaks, to be used as

using Peaks, Plots
t = 0:1/100:1
y = 2*sin.(5*t)+3*sin.(10*t)+2*sin.(30*t)
pks, vals = findmaxima(y)
pks, proms = peakproms!(pks, y; minprom=1)
plt = peaksplot(t, y, peaks=pks)
# or `peaksplot(t, y, peaks=pks, maxima=false)` to plot minima

peaks_prom_width

Only RecipesBase is added as a (very lightweight) dependency, Plots.jl must be installed by the user, or will be installed when running the unit tests.

Heavily inspired by JuliaNLSolvers/LsqFit.jl#180

PS: Thanks for this great package, it has served me extensively in the past :)

@halleysfifthinc halleysfifthinc added the enhancement New feature or request label Sep 21, 2022
Henning Labuhn and others added 9 commits October 18, 2022 14:50
(roughly in order of importance)

- Change prominence/width lines to be kwarg flag based
- For prominence lines only connect horizontal line to minima/maxima
  used for prominence calc
- Autodetect maxima vs minima
- Reorder series to change stacking of lines (prefer scatter points to
  be on top)
- Reuse NaN arrays
- Tweak visual characteristics of series
@halleysfifthinc
Copy link
Owner

Thanks for the PR! I rebased on master and made some tweaks to the interface and presentation.

The tests will need to be updated but I think that is all that is needed to merge. I plan on updating the tests tomorrow unless you get to them first! 😁

@MrHenning
Copy link
Contributor Author

Nice! 🙂
I'll try to update the test later today!

@MrHenning
Copy link
Contributor Author

@halleysfifthinc I updated the test, but the horizontal prominence lines are broken in the plot.
So I removed the drop_irrelevant_side method, since it works better without it ^^
I updated the README.md and plots/ as well.

If you don't agree with the changes, feel free to revert them, or you can tell me what to do 🙂

Copy link
Owner

@halleysfifthinc halleysfifthinc left a comment

Choose a reason for hiding this comment

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

Any ideas how we could avoid unnecessarily duplicating series when making the extrema figure?

@halleysfifthinc
Copy link
Owner

halleysfifthinc commented Oct 19, 2022

This is why I prefer dropping the unneeded side from the prominence lines:

image

I'm also not a fan of the left side of the width line just floating, but I don't know what an equivalent solution would be.

@MrHenning
Copy link
Contributor Author

ah ok I understand 😅
I'll revert the change I made

@halleysfifthinc
Copy link
Owner

One last question: I'm not familiar with the Plots ecosystem (I use PlotlyJS directly). Is a subject-verb phrasing (e.g. peaksplot) a common naming convention for recipes? I would otherwise prefer a more imperative, verb-subject phrasing (i.e. plotpeaks).

@MrHenning
Copy link
Contributor Author

I would say it’s the more common order, seeStatsPlots.jl or GraphRecipes.jl

But I have no strong opinion on this. plotpeaks indeed sounds more intuitive if I think about it.

@halleysfifthinc
Copy link
Owner

Hmm I think a key difference compared to plots in StatsPlots or GraphRecipes is that those are well-known named kinds of plots with proper names, whereas in this case I don't think that a "peaks" plot is a distinct/common named plot format. I'll switch it to plotpeaks and merge.

FYI There are another couple of updates I have planned before tagging v0.5.0, so I won't be tagging/releasing immediately.

@halleysfifthinc halleysfifthinc merged commit 7827ad5 into halleysfifthinc:master Oct 19, 2022
@MrHenning
Copy link
Contributor Author

Ok, no hurry from my side!

Thanks for the review!!! 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants