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 contourplot #199

Closed
wants to merge 12 commits into from
Closed

Add contourplot #199

wants to merge 12 commits into from

Conversation

t-bltg
Copy link
Member

@t-bltg t-bltg commented Jan 12, 2022

https://github.com/JuliaGeometry/Contour.jl is a small requirement, since it only depends on StaticArrays.

See examples at JuliaPlots/Plots.jl#4031.

The new example from README.md now looks like this (gaussian profile):
contourplot1

tiny benchmark

using UnicodePlots, BenchmarkTools

main() = begin
  x = -3:.1:3
  y = -7:.1:3
  X = repeat(x', length(y), 1)
  Y = repeat(y, 1, length(x))
  g(x, y, x₀=0, y₀=-2, σx=1, σy=2) = exp(-((x - x₀) / 2σx)^2 - ((y - y₀) / 2σy)^2)
  z = map(g, X, Y)
  @btime heatmap($z)  #   15.090 ms (197665 allocations: 9.45 MiB)
  @btime contourplot($x, $y, $z)  #   2.833 ms (29227 allocations: 1.18 MiB)
end

main()

@codecov-commenter
Copy link

codecov-commenter commented Jan 12, 2022

Codecov Report

Merging #199 (a49f96e) into master (65b9368) will decrease coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #199      +/-   ##
==========================================
- Coverage   99.82%   99.74%   -0.09%     
==========================================
  Files          23       24       +1     
  Lines        1164     1179      +15     
==========================================
+ Hits         1162     1176      +14     
- Misses          2        3       +1     
Impacted Files Coverage Δ
src/canvas/heatmapcanvas.jl 96.15% <ø> (-2.13%) ⬇️
src/colormaps.jl 100.00% <ø> (ø)
src/interface/barplot.jl 100.00% <ø> (ø)
src/canvas.jl 100.00% <100.00%> (ø)
src/canvas/asciicanvas.jl 100.00% <100.00%> (ø)
src/canvas/blockcanvas.jl 100.00% <100.00%> (ø)
src/canvas/braillecanvas.jl 98.07% <100.00%> (ø)
src/canvas/densitycanvas.jl 100.00% <100.00%> (ø)
src/canvas/dotcanvas.jl 100.00% <100.00%> (ø)
src/canvas/lookupcanvas.jl 100.00% <100.00%> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65b9368...a49f96e. Read the comment docs.

@t-bltg t-bltg force-pushed the contour branch 7 times, most recently from 6477963 to f5264eb Compare January 12, 2022 22:05
@t-bltg t-bltg changed the title Add contour support Add contourplot support Jan 13, 2022
@t-bltg t-bltg force-pushed the contour branch 2 times, most recently from 69d5fd5 to be6ccdb Compare January 13, 2022 12:14
@t-bltg t-bltg force-pushed the contour branch 4 times, most recently from bccc10c to c663f61 Compare January 13, 2022 13:00
@t-bltg t-bltg changed the title Add contourplot support Add contourplot Jan 13, 2022
Copy link
Collaborator

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

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

It's almost impossible to review a 1000-lines PR with arbitrary unorganized commits and changes so I only left a few comments on changes I think should be reverted or separated. There are still many out there that I didn't comment.

It would be clearer to separate the visible and blend changes in separate PR(s), and then make this countourplot PR an incremental feature on top of those PRs.

FWIW, for a clean git history: it makes no sense to record how you evolve into the current status, instead, it makes sense to record how functionalities are built step by step. Thus if you add something in the first commit and then immediately delete it in the second commit, it should be purged from the git history.

src/canvas/densitycanvas.jl Show resolved Hide resolved
src/canvas.jl Show resolved Hide resolved
src/colormaps.jl Outdated Show resolved Hide resolved
src/common.jl Show resolved Hide resolved
src/common.jl Show resolved Hide resolved
src/graphics/boxgraphics.jl Show resolved Hide resolved
@t-bltg
Copy link
Member Author

t-bltg commented Jan 17, 2022

Thanks for taking the time to reviewing this. The workflow is difficult since on this branch, I needed some colormap changes, and the integration / testing within Plots.jl is best if I apply the changes in a single branch, but I hear the concern on reviewing my PRs. Developing using multiple branches is also hard to follow, and wastes development time. Sorry about the code style changes, bad practice indeed. I will rebase the whole thing and split PRs.

@t-bltg
Copy link
Member Author

t-bltg commented Jan 17, 2022

Superseded by #204.

@t-bltg t-bltg closed this Jan 17, 2022
@t-bltg t-bltg deleted the contour branch January 17, 2022 16:58
@t-bltg t-bltg mentioned this pull request Jan 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FR] stand-alone version of printcolorbarrow to draw a colorbar in isolation
3 participants