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

Modification of plot2D to allow filled characters #152

Open
lmsimp opened this issue Oct 4, 2024 · 3 comments
Open

Modification of plot2D to allow filled characters #152

lmsimp opened this issue Oct 4, 2024 · 3 comments

Comments

@lmsimp
Copy link
Collaborator

lmsimp commented Oct 4, 2024

I would like to add functionality to plot2D to allow the use of filled point characters. Currently, it is not easy to straightforward to implement with ... which is passed to points in the plot2D code.

I would explicitly like to add an argument bg to plot2D (as it is used in points) and add functions getStockbg, setStockbg to work in the same way we currently have for col and setting marker colours.

I have already an amended version of plot2D I use that allows this and would like to submit a PR to update the version we have in pRoloc.

As discussed with you at Bioc eventually we may default to using ggplot for generating maps but plot2D is still very much used day-to-day in the lab and I think it is a great function that works out of the box within the pRoloc workflow.

Are you happy for me to add this and submit a PR? I will also add appropriate unit tests.

CC @Charl-Hutchings

@lgatto
Copy link
Owner

lgatto commented Oct 9, 2024

Does your update break any existing code? If not, then, I think it could be added. If there more risky changes, unit tests that tests the previous and new behaviour would be good.

@lmsimp
Copy link
Collaborator Author

lmsimp commented Oct 11, 2024

No existing code is broken. I think all is okay. I will check the vignettes.

I want to also change some of the default behaviour such as the call to grid and change it from grid = TRUE to grid = FALSE. Can I just change this or do I need some interim step to warn users the default behaviour has changed?

In any case, I will wait for @Charl-Hutchings to submit her PR for markers next week before adding this to a branch and submitting a PR.

@lgatto
Copy link
Owner

lgatto commented Oct 14, 2024

Not that (some of) the plot2D will hopefully become obsolete after the refactoring to SE, as it'll be possible to easily get a long table to use ggplot() on the fly.

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

No branches or pull requests

2 participants