Skip to content

Conversation

@t-bltg
Copy link
Member

@t-bltg t-bltg commented Jun 20, 2022

I feels more natural in julia to store the canvas with rows -> plot height and cols -> plot width. However, since we print plots row by row in a terminal, it's inefficient to iterate over the columns of a grid row.

The previous solution was to use grid[width, height], but you have to mentally transpose the canvas for writing algorithms. I solve this by transposing the grid on creation yielding grid[height, width] so that we can index using grid[row, col] and retain the performance when iterating over columns of a given row (printing). nrows(c) mapping to size(c.grid, 1) is more easy mentally.

Thus, high level functions lines!, points!, pixel!, char_point!, annotate! still retain the (x, y) order in their arguments, but the grid and colors access is now swapped: grid[y, x] being more intuitive.

Follow up of #267.

  • Add Changelog file
  • Add Developer Notes section

All changes:

  • swap (w, h) on canvas grid and colors - breaking change for canvas creation;
  • rename printrow to print_row (breaking since exported);
  • remove deprecated declarations and corresponding tests (breaking);
  • unexport canvas related functions (low level, should use import UnicodePlots: lines!, ...);
  • rename zscale to dscale for densityplot (ok since densityplot: support zscale - cleanup - immutability #271 not yet released);
  • huge (ridiculous) performance improvement for displaying colormap canvases on stdout by buffering i/o (see ex below);
  • add array keyword to heatmap for matrix display in array convention (origin at NW, y pointing downwards, x pointing to the right);
  • remove functor support (if haven't seen any real example of usage, and is only partially supported);
  • allow lineplot(x::Vector, y::Matrix) for plotting multiple series mapping to matrix columns (same for scatterplot);
  • allow width = :auto and height = :auto for creating a plot based on the current terminal size (fix Auto-resize based on terminal size #264);
  • add ColorSchemes.jl dependency, and remove all hard-coded colormap tables (no performance impact);
  • reduce number of exported symbols to only high level methods;
  • add head_tail_frac for lineplot using head_tail;
  • add hline! and vline!;
  • remove spy default title (redundant);
  • more code simplification / unification.
main() = display(heatmap(collect(1:150) * collect(1:150)', width = 150, height = 150))

main()  # compile
@time main()
1.812467 seconds (994.36 k allocations: 35.468 MiB)  # before (no buffering)
0.134729 seconds (635.64 k allocations: 25.404 MiB)  # pr (~14 faster)

@t-bltg
Copy link
Member Author

t-bltg commented Jun 20, 2022

@johnnychen94, I'm pinging you since these are breaking changes and I'm guessing that this will require a major bump. Please comment if any change is required.
I think there are very few people working with explicit Canvases instead of Plots and using printrow, if any, meaning that these changes should be transparent to the majority of the end users.
The deprecation warnings have been there for a long time: I think it's time to remove them from the code.

EDIT: I've rebased this PR

@codecov-commenter
Copy link

codecov-commenter commented Jun 20, 2022

Codecov Report

Merging #272 (f9e0e8d) into master (df510da) will decrease coverage by 0.05%.
The diff coverage is 99.83%.

❗ Current head f9e0e8d differs from pull request most recent head 81dba3f. Consider uploading reports for the commit 81dba3f to get more accurate results

@@             Coverage Diff             @@
##            master     #272      +/-   ##
===========================================
- Coverage   100.00%   99.94%   -0.06%     
===========================================
  Files           29       29              
  Lines         1631     1700      +69     
===========================================
+ Hits          1631     1699      +68     
- Misses           0        1       +1     
Impacted Files Coverage Δ
src/description.jl 100.00% <ø> (ø)
src/interface/densityplot.jl 100.00% <ø> (ø)
src/interface/isosurface.jl 100.00% <ø> (ø)
src/interface/polarplot.jl 100.00% <ø> (ø)
src/common.jl 99.53% <98.70%> (-0.47%) ⬇️
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 100.00% <100.00%> (ø)
src/canvas/densitycanvas.jl 100.00% <100.00%> (ø)
... and 40 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 df510da...81dba3f. Read the comment docs.

@t-bltg t-bltg changed the title Swap (w, h) - rename printrow - remove deprecations Swap (w, h) - rename printrow - remove deprecated Jun 20, 2022
@t-bltg t-bltg force-pushed the transpose branch 3 times, most recently from abadc7e to e36c483 Compare June 21, 2022 09:51
@t-bltg t-bltg mentioned this pull request Jun 21, 2022
@t-bltg t-bltg force-pushed the transpose branch 21 times, most recently from 1949be1 to 947b5f3 Compare June 22, 2022 11:51
@t-bltg t-bltg force-pushed the transpose branch 7 times, most recently from f1e288e to dd372c5 Compare June 23, 2022 18:25
@t-bltg t-bltg force-pushed the transpose branch 4 times, most recently from 9c9296f to 1cf453b Compare June 23, 2022 20:24
@t-bltg
Copy link
Member Author

t-bltg commented Jun 23, 2022

I will probably merge this PR tomorrow and release 3.0.0, unless objections / comments.

@t-bltg t-bltg force-pushed the transpose branch 11 times, most recently from 444cb7b to 1c548b4 Compare June 24, 2022 09:05
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.

Auto-resize based on terminal size

3 participants