feature/rect #1263

CiaranOMara · 2019-03-24T10:01:51Z

Develops RectangularGeometry separately from RectangularBinGeometry. It was suggested in #1176 that RectangularGeometry could be a base geometry.

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

Develops RectangularGeometry separately from RectangularBinGeometry.

Breaking changes

Geometry Geom.rect no longer reduces height or width by theme.bar_spacing. This change leads to a regression in the Geom.rectbin test where Geom.rect is used.

codecov-io · 2019-03-24T10:37:03Z

Codecov Report

Merging #1263 into master will decrease coverage by 0.13%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #1263      +/-   ##
==========================================
- Coverage   90.32%   90.18%   -0.14%     
==========================================
  Files          39       40       +1     
  Lines        4060     4281     +221     
==========================================
+ Hits         3667     3861     +194     
- Misses        393      420      +27

Impacted Files	Coverage Δ
src/geometry.jl	`75% <ø> (ø)`	⬆️
src/geom/rectbin.jl	`80% <100%> (-7.5%)`	⬇️
src/geom/rect.jl	`82.5% <82.5%> (ø)`
src/poetry.jl	`90.74% <0%> (-5.26%)`	⬇️
src/geom/label.jl	`96.26% <0%> (-2.74%)`	⬇️
src/geom/beeswarm.jl	`90% <0%> (-2.65%)`	⬇️
src/guide/keys.jl	`84.03% <0%> (-1.32%)`	⬇️
src/scale/scales.jl	`88.7% <0%> (-1.13%)`	⬇️
src/guide.jl	`90.64% <0%> (-0.27%)`	⬇️
... and 17 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 b7e8693...8d72b85. Read the comment docs.

bjarthur · 2019-03-24T12:51:25Z

great to have this revived!

would you care to take the extra step of deleting function render(geom::RectangularBinGeometry... and refactoring Stat.histogram2d and Stat.rectbin to use Geom.RectangularGeometry?

src/geom/rectbin.jl

bjarthur · 2019-03-30T20:16:28Z

re. the breaking change with Theme.bar_spacing...

what do you think of adding two fields to RectangularGeometry: vpadding::Union{Symbol,Measure} and a similarly typedhpadding, both of which default to the symbol :theme_bar_spacing. then in render, if they are equal to :theme_bar_spacing you subtract theme.bar_spacing, otherwise you subtract the Measured value.

in #1264 you could then define band as a call to RectangularGeometry with both paddings set to 0mm.

would all this work to not break backwards compatibility and to retain the flexibility for bands?

Mattriks · 2019-03-31T00:48:55Z

src/geom/rect.jl

+
+ return compose!(
+ context(),
+ Compose.polygon(polys, geom.tag),


I ran into issues using polygons where one axis was a date/datetime type in: GiovineItalia/Compose.jl#338 (comment). You'll need to include tests of that with this PR.

@Mattriks could you please elaborate on this request for a test?

Mattriks · 2019-03-31T00:53:43Z

NEWS.md

@@ -1,6 +1,9 @@

 This is a log of major changes in Gadfly between releases. It is not exhaustive.
 Each release typically has a number of minor bug fixes beyond what is listed here.
+# Version 1.x


Gadfy hasn't reached 1.1 yet, so this news item can go under the # Version 1.1.0 heading below. And include the (PR number)

Mattriks · 2019-03-31T01:11:14Z

Since this PR updates the render function for rectangles, it would be good to add support for the alpha aesthetic, so rectangles can work with different color and alpha aesthetic groupings. PointGeometry, PolygonGeometry, and RibbonGeometry all currently support alpha, so you can look at their render functions for examples of how to implement.

CiaranOMara · 2019-03-31T11:23:00Z

@bjarthur, a counter to your reply regarding theme_bar_spacing.

What about adding a hook that gets called between the application of statistics and rendering geometries?

In this case the hook signature could be something like apply_theme_transformations!(geom::RectangularGeometry, stat::RectbinStatistic, aes::Gadfly.Aesthetics, theme::Gadfly.Theme).

bjarthur · 2019-04-02T11:19:19Z

src/Gadfly.jl

+
+ aes.xmax = aes.xmax .* cx .- theme.bar_spacing
+ aes.ymax = aes.ymax .* cy .- theme.bar_spacing
+end


@Mattriks @tlnagy thoughts on this proposed new apply_theme_transformations! addition to the existing framework?

seems okay to me, but it's not clear what advantages it confers to just handling this case in the corresponding render function.

are there other cases which could be refactored like this?

certainly the @info and @warn need to go.

I naively wondered if it were possible to handle default colour and alpha with this sort of pattern.

I'm not liking apply_theme_transformations!, starting with the strange looping in render_prepared(). Although render_prepared has an argument layer_stats, that argument is not used in render_prepared, and there is no reason for it to be: the applying of Stats is done by render_prepare().

This sort of "code to handle common cases" should be done once there is a good idea of what those common cases are. For example, over time the default_aes list (which appears at the start of render functions for polygon, ribbon, point etc) has become more similar, and this is where "unifying code" might be useful. (Also if someone wanted to do that, it should be done in a separate PR).

Regarding the strange looping in render_prepared(), I picked a spot between application of statistics and rendering where all of the variables required to demonstrate the concept were readily available; it was by no means a suggestion of final implementation. Commit bac3a29 places the apply_theme_transformations! methods where I'd expect to find them. However, the commit does not develop the hook call.

In developing a new apply_ function, I would like to see a design outline like this:

# Minimal Gadfly # function render_prepare datas = Gadfly.Data(x=1:10, y=rand(10), xend=1:10, yend=zeros(10)) scales=Dict{Symbol,Gadfly.ScaleElement}(:x=>Scale.x_continuous(), :y=>Scale.y_continuous()) aes = Scale.apply_scales(collect(values(scales)), datas) coord = Coord.cartesian() stats = Gadfly.StatisticElement[Stat.xticks(), Stat.yticks()] Stat.apply_statistics(stats, scales, coord, aes[1]) # stats have been implemented, and the stats vector is no longer needed # function render_prepared plot_context = Coord.apply_coordinate(coord, aes, scales) layer_themes = [Theme(default_color="green"), Theme(default_color="orange")] geom_ctxs = render.([Geom.point(), Geom.hair()], layer_themes, aes) guide_ctxs = render.([Guide.xticks(), Guide.yticks()], [Theme()], aes) compose!(plot_context, (context(), geom_ctxs...)) tbl = Guide.layout_guides(plot_context, coord, Theme(), [x[1] for x in guide_ctxs]...) p = compose!(context(), tbl)

with an explanation about why I want to add a new apply_ function to the above code. Then I would ask about timings. Also, I would prefer to see such a new function in a separate PR.

I think it's better for those two geometries (RectGeo and RectBinGeo) to evolve separately (let's give them the chance to do that).

@bjarthur, as of the rollback to 8339918 there is a regression in testscripts/rectbin.jl where Geom.rect and Geom.rectbin intertwine. I think Geom.rect should be removed from the rectbin test. I'll have a look at the documentation and make sure the geometries are separated there too. However, this regression alerts me to an important question: what is an appropriate overlap between a max and next min that share the same value? In the current implementation, is it the stroke that overlaps and not the fill (which I think is appropriate), or does the fill overlap? I'm not sure of the specifics, but suppose it depends on Compose.jl and the various backends. I can foresee us encountering undesired artifacts when applying alpha to Geom.rect if the nuance of overlap is not correct.

Happy to squash commits once everything looks good.

@Mattriks, the rollback undoes the changes to Geom.rectbin to allow it to evolve separately.

is the regression whether the fill and stroke overlap or not? is there no way to fix it?

@bjarthur, I'm confident the regression is due to the exclusion of theme.bar_spacing from the new Geom.rect implementation.

Below is the rect.png test output from beee870, which is a little fuzzy about the edges.

Below is the rect.pdf test output zoomed in on (0.5, 0.5) of the plot on the left side.

Given these new images, I do not think there is any overlap of fill in the current implementation. However, stroke is currently stroke(nothing).

In terms of regressions, they're isolated to Geom.rect. No other geometries derive from Geom.rect, other geometries are deriving from Geom.rectbin.

Do you think the exclusion of theme.bar_spacing from the new Geom.rect makes sense?

i don't think we can tolerate any regressions until we decide it's time to release 2.0. if you want to keep this as is, we'll have to put a milestone on it and save it for later.

having a way to slightly adjust the dimensions of the rectangles seems generally useful to me. Theme.bar_spacing doesn't seem the right way to go about it though since that sounds specific to Geom.bar. but i'd suggest sticking with it for now, and perhaps filing an issue with a milestone 2.0 tag to remind us to change it later.

CiaranOMara mentioned this pull request Mar 24, 2019

HBandGeometry and VBandGeometry #1176

Closed

6 tasks

CiaranOMara force-pushed the feature/rect branch from 53e456f to 62ee5f6 Compare March 25, 2019 00:36

bjarthur reviewed Mar 30, 2019

View reviewed changes

src/geom/rectbin.jl Outdated Show resolved Hide resolved

Mattriks reviewed Mar 31, 2019

View reviewed changes

CiaranOMara force-pushed the feature/rect branch 2 times, most recently from db70c10 to df934ec Compare March 31, 2019 13:52

bjarthur reviewed Apr 2, 2019

View reviewed changes

CiaranOMara force-pushed the feature/rect branch 2 times, most recently from bac3a29 to 8339918 Compare April 4, 2019 23:28

CiaranOMara force-pushed the feature/rect branch from dc3805c to beee870 Compare October 13, 2019 21:57

CiaranOMara force-pushed the feature/rect branch from beee870 to d9b626c Compare November 22, 2019 08:53

Mattriks mentioned this pull request Nov 23, 2019

Alpha in SVGJS #1353

Closed

CiaranOMara added 7 commits February 15, 2020 21:18

Develop RectangularGeometry

9266d3d

Correct visibility logic

a769495

Use Compose.polygon

3b529ff

Develop RectangularGeometry return

134e935

Separate Geom.rect and Geom.rectbin gallery examples

1185c44

Remove Geom.rect from Geom.rectbin test

a7fce1f

Develop rect test to determine overlap

8d72b85

CiaranOMara force-pushed the feature/rect branch from d9b626c to 8d72b85 Compare February 15, 2020 10:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature/rect #1263

feature/rect #1263

CiaranOMara commented Mar 24, 2019 •

edited

codecov-io commented Mar 24, 2019 •

edited

bjarthur commented Mar 24, 2019

bjarthur commented Mar 30, 2019 •

edited

Mattriks Mar 31, 2019

bjarthur Apr 5, 2019

Mattriks Mar 31, 2019

Mattriks commented Mar 31, 2019

CiaranOMara commented Mar 31, 2019

bjarthur Apr 2, 2019

CiaranOMara Apr 2, 2019

Mattriks Apr 2, 2019 •

edited

CiaranOMara Apr 3, 2019

Mattriks Apr 4, 2019 •

edited

Mattriks Apr 5, 2019

CiaranOMara Apr 8, 2019 •

edited

bjarthur Apr 15, 2019

CiaranOMara Apr 17, 2019

bjarthur Apr 17, 2019

feature/rect #1263

Are you sure you want to change the base?

feature/rect #1263

Conversation

CiaranOMara commented Mar 24, 2019 • edited

Contributor checklist:

Proposed changes

Breaking changes

codecov-io commented Mar 24, 2019 • edited

Codecov Report

bjarthur commented Mar 24, 2019

bjarthur commented Mar 30, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mattriks commented Mar 31, 2019

CiaranOMara commented Mar 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mattriks Apr 2, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mattriks Apr 4, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CiaranOMara Apr 8, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CiaranOMara commented Mar 24, 2019 •

edited

codecov-io commented Mar 24, 2019 •

edited

bjarthur commented Mar 30, 2019 •

edited

Mattriks Apr 2, 2019 •

edited

Mattriks Apr 4, 2019 •

edited

CiaranOMara Apr 8, 2019 •

edited