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

Added ability for intercept in geom.hair to take vector to function as lolipop #1519

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hewsond
Copy link

@hewsond hewsond commented Feb 18, 2021

Chainged Geom.hair
Documented hair as lolipop in geometries documentation
Added Geom.hair update as lolipop to news
Added tests for Geom.hair lolipop

Fixes #XXX

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

Use length check on intercept in Geom.hair to determine if it is same length as number of points want to draw hairs for.

If it is same length, don't fill end point vector, just use the supplied vector.

hair

Doc building issues

I've added it to the test files and above you can see the output, however when i try to build the documentation I get an error:
MethodError: no method matching isless(::Array{Float64,1}, ::Int64)

Though it isn't the only @example block that gives some errors.

…s lolipop

Documented hair as lolipop in geometries documentation
Added Geom.hair update as lolipop to news
Added tests for Geom.hair lolipop
@Mattriks
Copy link
Member

Another possibility here is changing the syntax to plot(x=x, y=x.^2, intercept=(x.^2)./2, Geom.hair, Geom.point), and allow intercept to take vectors of length one (or more). I've got a PR coming soon which refactors some internal code and is relevant to making that switch.

@hewsond
Copy link
Author

hewsond commented Feb 19, 2021

I'm not an expert on the whole grammar of graphics thing but IMO the intercept argument as you suggest does sound much nicer and cleaner.

I'm happy to have at re-basing to work in that way once you've done the refactor if required.

@bjarthur
Copy link
Member

@Mattriks did you finished the aforementioned PR?

@Mattriks
Copy link
Member

No unfortunately. I think I was referring to #1520 (which I opened on the same day as the posts above), or a subsequent PR. I do hope to return to #1520 sometime!

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

3 participants