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

Simplify build_layout logic #3609

Merged
merged 1 commit into from
Jul 5, 2021
Merged

Conversation

t-bltg
Copy link
Member

@t-bltg t-bltg commented Jul 4, 2021

As per

# TODO... much of the logic overlaps with the method above... can we merge?
.

src/layouts.jl Show resolved Hide resolved
@t-bltg
Copy link
Member Author

t-bltg commented Jul 4, 2021

I'm merging #3598, because JuliaPlots/PlotReferenceImages.jl#100 has been merged.

@t-bltg t-bltg closed this Jul 4, 2021
@t-bltg t-bltg reopened this Jul 4, 2021
@t-bltg
Copy link
Member Author

t-bltg commented Jul 4, 2021

We hit JuliaStats/Distributions.jl#1358.

Fixed

@t-bltg t-bltg closed this Jul 4, 2021
@t-bltg t-bltg reopened this Jul 4, 2021
@codecov
Copy link

codecov bot commented Jul 4, 2021

Codecov Report

Merging #3609 (753df0e) into master (014b86a) will increase coverage by 1.44%.
The diff coverage is 76.13%.

❗ Current head 753df0e differs from pull request most recent head 5eb62ab. Consider uploading reports for the commit 5eb62ab to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3609      +/-   ##
==========================================
+ Coverage   63.72%   65.16%   +1.44%     
==========================================
  Files          27       27              
  Lines        6566     6643      +77     
==========================================
+ Hits         4184     4329     +145     
+ Misses       2382     2314      -68     
Impacted Files Coverage Δ
src/recipes.jl 58.09% <0.00%> (-0.57%) ⬇️
src/plot.jl 67.20% <7.40%> (-15.80%) ⬇️
src/animation.jl 38.20% <50.00%> (ø)
src/components.jl 61.89% <72.72%> (+5.18%) ⬆️
src/utils.jl 52.58% <84.61%> (+0.07%) ⬆️
src/backends/pgfplotsx.jl 60.94% <85.71%> (+1.61%) ⬆️
src/backends/gr.jl 89.00% <96.42%> (+1.48%) ⬆️
src/args.jl 72.92% <100.00%> (+0.46%) ⬆️
src/axes.jl 84.02% <100.00%> (+7.87%) ⬆️
src/examples.jl 61.76% <100.00%> (+0.57%) ⬆️
... and 14 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 3f110d6...5eb62ab. Read the comment docs.

@t-bltg t-bltg requested a review from BeastyBlacksmith July 4, 2021 19:36
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.

Whats up with the benchmarks? Can you also re-enable the StatsPlots tests, if they are fixed?

@t-bltg
Copy link
Member Author

t-bltg commented Jul 5, 2021

Whats up with the benchmarks?

Which benchmarks ?

Can you also re-enable the StatsPlots tests, if they are fixed?

That won't work at the moment see the conditions listed at the tracker #3611.

@BeastyBlacksmith You're right, I think we can't reverse the fix now, CI passes in #3611.

@t-bltg t-bltg closed this Jul 5, 2021
@t-bltg t-bltg reopened this Jul 5, 2021
@t-bltg t-bltg closed this Jul 5, 2021
@t-bltg t-bltg reopened this Jul 5, 2021
@t-bltg
Copy link
Member Author

t-bltg commented Jul 5, 2021

@BeastyBlacksmith, could you explain to me in 2secs where the benchmark results can be seen ? (not handy to scroll the logs).
EDIT: the github-actions bot seems to post benchmark results on some branches only, is there a setting to make it post here for example ?

I'm not familiar with those: I believe they are used to measure performance regression somehow ? What is our policy for saying a benchmark is OK or not ?

@t-bltg t-bltg closed this Jul 5, 2021
@t-bltg t-bltg reopened this Jul 5, 2021
@t-bltg
Copy link
Member Author

t-bltg commented Jul 5, 2021

Whats up with the benchmarks?

LGTM 💯

Results

ID time ratio memory ratio
["display"] 0.00 (5%) ✅ 0.00 (1%) ✅
["load_plot_display"] 0.97 (5%) 0.00 (1%) ✅
["plot"] 0.72 (5%) ✅ 1.00 (1%)

@t-bltg t-bltg merged commit 09356ee into JuliaPlots:master Jul 5, 2021
@t-bltg t-bltg deleted the simplify_layouts branch July 5, 2021 10:50
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.

2 participants