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 stephist #2408

Merged
merged 15 commits into from
Jan 28, 2023
Merged

add stephist #2408

merged 15 commits into from
Jan 28, 2023

Conversation

Moelf
Copy link
Contributor

@Moelf Moelf commented Nov 8, 2022

Description

helps #368

Looks stupid right now because I just copied the Hist, probably can be simplified, welcome comments

Type of change

Delete options that do not apply:

  • New feature (non-breaking change which adds functionality)

Checklist

  • Added an entry in NEWS.md (for new features and breaking changes)
  • Added or changed relevant sections in the documentation
  • Added unit tests for new algorithms, conversion methods, etc.
  • Added reference image tests for new plotting functions, recipes, visual options, etc.

@Moelf
Copy link
Contributor Author

Moelf commented Nov 8, 2022

julia> f = stephist(rand(10000))
FigureAxisPlot()

image

src/stats/hist.jl Outdated Show resolved Hide resolved
@cossio
Copy link
Contributor

cossio commented Dec 14, 2022

I was just looking for this, would be great to have. Thanks, I hope this gets merged eventually.

@Moelf
Copy link
Contributor Author

Moelf commented Dec 14, 2022

if people are feeling okay with the implementation I can start adding News, Docs and test samples

@jkrumbiegel
Copy link
Member

Would probably make sense to factor out common functionality with hist

@Moelf
Copy link
Contributor Author

Moelf commented Dec 14, 2022

What's the core maintainers' opinion on picking up a histogram package as a dependency? I've grown tired of re-writing haphazard binning logic in multiple places :(

@jkrumbiegel
Copy link
Member

You mean a package beyond StatsBase's histogram fitting?

@Moelf
Copy link
Contributor Author

Moelf commented Dec 15, 2022

this is ready I think. Looks like we don't do reference image for this kind of non basic plots? I checked waterfall and tricontourf PRs.

Let me know if we want anything else

@jkrumbiegel
Copy link
Member

Looks like we don't do reference image for this kind of non basic plots?

Would still be great if you added a reference image, could be one image with a couple of subplots that expose different settings (ideally thick lines so that the visual differ would pick up on it if anything broke).

Also, the docs build is currently broken because of an automatic value being passed too far into the GLMakie backend it seems?

@Moelf
Copy link
Contributor Author

Moelf commented Dec 17, 2022

Do you have an example of PR adding references imagine?

@jkrumbiegel
Copy link
Member

jkrumbiegel commented Dec 17, 2022

The PRs don't add the images themselves, just the code. You can check them in the ReferenceTests folder, all the @reference macros

@Moelf
Copy link
Contributor Author

Moelf commented Jan 7, 2023

Am I doing it correctly with reference?

@Moelf
Copy link
Contributor Author

Moelf commented Jan 7, 2023

can't seem to find which automatic is too far

@lazarusA
Copy link
Contributor

@Moelf this looks like almost there. Maybe a final sprint to make it possible.

@Moelf
Copy link
Contributor Author

Moelf commented Jan 26, 2023

yeah I just don't understand what Automatic() is getting propagated, since I pretty much took a subset from regular histogram

end

# plot the values, not the observables, to be in control of updating
bp = stairs!(plot, points[]; plot.attributes..., color=color)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this just needs to be

attr = copy(plot.attributes)
pop!(attr, :weights)
bp = stairs!(plot, points[]; attr..., color=color)

if pass-through of automatic is the problem

@Moelf
Copy link
Contributor Author

Moelf commented Jan 27, 2023

https://github.com/MakieOrg/Makie.jl/actions/runs/4018541242/jobs/6904286091#step:7:613

I think we have made progress, now only the Reference saving is not working, due to what seems to be because it's trying to save Observable?

@SimonDanisch
Copy link
Member

I cleaned up the attributes! I think you copied a few attributes from hist, without using them?
If I removed anything that wasn't supposed to be removed I can revert the commit ;)
But the error was due to passing label_font to stairs, which doesn't support that attribute

@SimonDanisch SimonDanisch merged commit 8a2bc5d into MakieOrg:master Jan 28, 2023
@Moelf Moelf deleted the stephist branch January 28, 2023 18:58
@cossio
Copy link
Contributor

cossio commented Jan 29, 2023

Thanks for merging this. Could we tag a release containing this PR?

@asinghvi17
Copy link
Member

@cossio that'll be done in #2636

@cossio
Copy link
Contributor

cossio commented Jan 29, 2023

Fantastic, thank you.

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.

None yet

7 participants