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

feature/hvband #1264

Merged
merged 1 commit into from
Nov 24, 2019
Merged

feature/hvband #1264

merged 1 commit into from
Nov 24, 2019

Conversation

CiaranOMara
Copy link
Contributor

@CiaranOMara CiaranOMara commented Mar 24, 2019

This feature provides Geom.hband and Geom.vband geometries that address a need similar to that expressed in #1174. This pull request revives #1176.

Contributor checklist:

  • I've updated the documentation to reflect these changes
  • I've added an entry to NEWS.md
  • I've added and/or updated the unit tests
  • I've run the regression tests
  • I've squash'ed or fixup'ed junk commits with git-rebase
  • I've built the docs and confirmed these changes don't cause new errors

Proposed changes

  • Add Geom.hband geometry that has the same width properties of hline, but a vertical span specified by ymin and ymax.
  • Add Geom.vband geometry that has the same height properties of vline, but a horizontal span specified by xmin and xmax.
  • Allow elements of type Measure to pass through coord.jl and statistics.jl.

@CiaranOMara CiaranOMara mentioned this pull request Mar 24, 2019
6 tasks
@codecov-io
Copy link

codecov-io commented Mar 24, 2019

Codecov Report

Merging #1264 into master will increase coverage by 0.01%.
The diff coverage is 95.23%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1264      +/-   ##
=========================================
+ Coverage   90.59%   90.6%   +0.01%     
=========================================
  Files          37      38       +1     
  Lines        3945    3961      +16     
=========================================
+ Hits         3574    3589      +15     
- Misses        371     372       +1
Impacted Files Coverage Δ
src/geometry.jl 75% <ø> (ø) ⬆️
src/coord.jl 91.39% <100%> (ø) ⬆️
src/geom/hvband.jl 100% <100%> (ø)
src/statistics.jl 96.88% <93.33%> (-0.06%) ⬇️

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 78e4c9a...cb916d1. Read the comment docs.

@CiaranOMara
Copy link
Contributor Author

@bjarthur, @Mattriks, and @tlnagy, what were the issues with this PR?

NEWS.md Outdated Show resolved Hide resolved
@Mattriks
Copy link
Member

I think the discussion about this was in #1176, and it looks like you didn't get back to this PR after splitting #1176. I think to finish this you just need to complete the checklist in you first post above. Have you redone the regression tests?

@CiaranOMara
Copy link
Contributor Author

I was able to do the regression tests with @davidanthoff's Rsvg.jl PR. Somethings pertaining to histogram and hexbin came up, but I don't think they're related to changes here. Let me know if you would like me to post the Diffs here.

src/coord.jl Show resolved Hide resolved
@bjarthur
Copy link
Member

this will be nice to have! thanks for this. what was different in the regression tests?

@CiaranOMara
Copy link
Contributor Author

These diffs appear to be due to minor variations in how the colours have rasterised. In my opinion, the newer version looks sharper.

colorful_hist
colorful_hist png

image

dodged_discrete_histogram_horizontal
dodged_discrete_histogram_horizontal png

image

dodged_discrete_histogram
dodged_discrete_histogram png

image

hexbin_dark
hexbin_dark png

image

hexbin
hexbin png

image

stacked_continuous_histogram
stacked_continuous_histogram png

image

stacked_discrete_histogram
stacked_discrete_histogram png

image

@bjarthur
Copy link
Member

i've seen these minor differences often before too. not sure how to complain. have never held up a PR because of it.

thanks!

@bjarthur bjarthur merged commit a6f89ce into GiovineItalia:master Nov 24, 2019
@CiaranOMara CiaranOMara deleted the feature/hvband branch February 15, 2020 10:59
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

4 participants