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/rect #1263

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

CiaranOMara
Copy link
Contributor

@CiaranOMara CiaranOMara commented Mar 24, 2019

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
Copy link

codecov-io commented Mar 24, 2019

Codecov Report

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

Impacted file tree graph

@@            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.

@CiaranOMara CiaranOMara mentioned this pull request Mar 24, 2019
6 tasks
@bjarthur
Copy link
Member

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 Outdated Show resolved Hide resolved
@bjarthur
Copy link
Member

bjarthur commented Mar 30, 2019

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?


return compose!(
context(),
Compose.polygon(polys, geom.tag),
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

NEWS.md Outdated
@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

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
Copy link
Contributor Author

@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).

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

aes.xmax = aes.xmax .* cx .- theme.bar_spacing
aes.ymax = aes.ymax .* cy .- theme.bar_spacing
end
Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@Mattriks Mattriks Apr 2, 2019

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@Mattriks Mattriks Apr 4, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@CiaranOMara CiaranOMara Apr 8, 2019

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.
rect

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

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.

4 participants