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] Gutters for Swarmplots #9

Merged
merged 11 commits into from
Mar 15, 2024
Merged

[FEATURE] Gutters for Swarmplots #9

merged 11 commits into from
Mar 15, 2024

Conversation

TheCedarPrince
Copy link
Collaborator

@TheCedarPrince TheCedarPrince commented Mar 12, 2024

Hey @asinghvi17,

Here's a rough cut implementation of adding gutters to the swarm plotting algorithm! It is very naive right now so curious about how we could optimize it a bit better.

Closes #8

Cheers!

~ tcp 🌳

@TheCedarPrince
Copy link
Collaborator Author

Hm. I also realized after implementing this that it currently only works with the horizontal direction. I'll have to come back to implement the other direction at some point soon.

Copy link
Member

@asinghvi17 asinghvi17 left a comment

Choose a reason for hiding this comment

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

Hey! This code looks good. One question that comes to mind: do we define a gutter in pixel space or data space? They both have advantages but data space seems a bit easier to tune. Either could be done.

src/recipe.jl Outdated Show resolved Hide resolved
src/recipe.jl Show resolved Hide resolved
src/recipe.jl Outdated Show resolved Hide resolved
src/recipe.jl Outdated Show resolved Hide resolved
src/recipe.jl Outdated Show resolved Hide resolved
src/recipe.jl Outdated Show resolved Hide resolved
src/recipe.jl Outdated Show resolved Hide resolved
src/recipe.jl Outdated Show resolved Hide resolved
@TheCedarPrince
Copy link
Collaborator Author

Hey @asinghvi17 , here's some thoughts after applying your suggestions:

One question that comes to mind: do we define a gutter in pixel space or data space? They both have advantages but data space seems a bit easier to tune.

I prefer data space as I think this would be more intuitive to explain in the documentation to new folks working with beeswarm. Pixel space I think would get in the way of folks who "just want to plot a beeswarm".

Yep that sounds good! We could also then overload the method for each algorithm, which neatly solves a couple of other issues with the algorithm not having access to the gutter.

In terms of an API, what are you imagining when you are talking about overloading and neatly solving issues? I wasn't completely following this train of thought.

Thanks Anshul! Let me know if you have any questions!

~ tcp 🌳

@asinghvi17
Copy link
Member

Thanks a lot Jacob! The implementation looks really good now!

I think we just need to account for direction - if direction is set to :x, for example, then the gutter would still remove points in the x direction which is not what we want. For this, I think we can just add if statements to (a) when we extract xs and (b) when we update the points (if direction == :x; Point2f(y, newx); else; Point2f(newx, y); end or so).

I get what you mean about gutters making more sense in dataspace, and agree.

My thought on pixelspace was that one could define gutter_threshold = 5*markersize and then have a histogram of guaranteed length maximum 5. That being said, this is a bit hard to reason about and adjust, and probably if you want to fine tune it should not be too difficult in data space.

In terms of API for gutterize - I was thinking something like this:

function gutterize!(point_buffer, algorithm::BeeswarmAlgorithm, gutter, side, direction)
    # implementation here
end

and then we wrap the changes you made here into this function.

Copy link
Member

@asinghvi17 asinghvi17 left a comment

Choose a reason for hiding this comment

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

I'll put some test cases together and upstream them!

src/recipe.jl Outdated Show resolved Hide resolved
src/recipe.jl Outdated Show resolved Hide resolved
src/recipe.jl Outdated Show resolved Hide resolved
src/recipe.jl Outdated Show resolved Hide resolved
src/recipe.jl Outdated Show resolved Hide resolved
Untested as of now but will push tests shortly.
@TheCedarPrince
Copy link
Collaborator Author

Brilliant @asinghvi17 ! Let me know when you are done tweaking this and then I can jump back on with implementing some more of what you are thinking.

Were you going to implement gutterize! or would you like me to?

@asinghvi17
Copy link
Member

No worries, I can get it in :)

docs/src/gutters.md Outdated Show resolved Hide resolved
@asinghvi17 asinghvi17 merged commit 3de71ef into main Mar 15, 2024
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.

Implement gutters/corrals
2 participants