Skip to content

Conversation

@ahl27
Copy link
Collaborator

@ahl27 ahl27 commented Jan 2, 2025

Adds a collection of functions that allow users to specify new color palettes to display XString objects. This solves #115.

Slight refactor to move global variables into a package-specific environment to remove some <<- calls.

Also allows for users to specify colors for BString objects if they'd like, though the default is still no coloring.

Adds some additional unit tests and documentation pages.

I'm not sure I love the update_X_palette format for the function names...maybe updateDNApalette or update_DNA_COLORED_LETTERS would be better? Unsure.

Example usage is below.

DNA_palette <- list(
  A=list(fg="blue",bg="black"),
  T=list(fg="red",bg='black'),
  G=list(fg='green',bg='black'),
  C=list(fg='yellow',bg='black')
)
AA_palette <- list(
  A=list(fg="white", bg="purple"),
  B=list(fg=rgb(1,1,1), bg='orange')
)
B_palette <- list(
  A=list(bg='green'),
  B=list(bg="red"),
  C=list(bg='blue'),
  D=list(fg="orange"),
  E=list(fg="yellow")
)
Screenshot 2025-01-02 at 13 19 44

@hpages
Copy link
Contributor

hpages commented Feb 18, 2025

@ahl27 Thanks Aidan. Please name the Rd file coloring.Rd (and start the file with \name{coloring}). As a general rule, functions implemented in foo.R should be documented in foo.Rd and have unit tests in test-foo.R. I find that this helps keep things organized and easy to find.

Also:

> DNA_palette <- list(
  A=list(fg="blue",bg="black"),
  G=list(fg='green',bg='black'),
  E=list(fg='yellow',bg='black')
)

> update_DNA_palette(DNA_palette)
Error in error("Invalid DNA/RNA codes specified.") : 
  could not find function "error"

Always spend a little bit of time trying to break your own code, it's a good exercise. Even better is to turn this exercise into unit tests.

Thanks again!

@ahl27
Copy link
Collaborator Author

ahl27 commented Feb 18, 2025

urgh, what a silly mistake...I'm getting better at catching them, but I guess I still have a ways to go.

Thanks for your feedback, I'll make these changes today or tomorrow!

@ahl27
Copy link
Collaborator Author

ahl27 commented Feb 20, 2025

Made the following fixes:

  • error messages are a little more informative
  • error messages now correctly call stop...I'm not sure why I always think that function is error. Maybe spending too much time in C :/
  • tests relating to coloring have been moved to test-coloring.R. They also now test sad path cases.
  • update_X_palette.Rd renamed to coloring.R. Documentation file is now more general and includes details on the coloring of all XString objects. This should make it easier to add more functions in the future.
  • update_B_palette now checks for valid entries (must be single characters, though it supports values from 0-255)

@hpages
Copy link
Contributor

hpages commented Feb 20, 2025

Thanks for all the improvements. Man page and unit tests look good!

Please add \alias{coloring} to the man page. This can help users (like me) find the man page if they can't remember the exact names of the functions documented in the man page.

Let's update the header of R/coloring.R to:

### =========================================================================
### XString Display Colors
### -------------------------------------------------------------------------

(resusing the title of your man page here), and remove this line (we'll probably forget to update it if we ever rename these functions or add new exported functions in the future):

### Only update_X_palette() methods are exported

Looks like there's code repetition in R/coloring.R that could easily be avoided, e.g. .add_dna_and_rna_colors, .add_aa_colors, and .add_bstring_colors repeat almost the same code. Same for update_DNA_palette and update_AA_palette. Can you try to refactor a little bit to avoid the repetition?

Thanks again,
H.

@ahl27
Copy link
Collaborator Author

ahl27 commented Feb 20, 2025

Great comments, I was too focused on extending the current codebase that I didn't realize it could use a refactor. Getting BStrings to work with it was a little odd, but I think overall it's more readable and maintainable this way. I think it's a good improvement to the coloring stuff!

Changes:

  • Header of R/coloring.R has been updated accordingly
  • BSTRING_COLORED_LETTERS is now B_COLORED_LETTERS for consistency with the other variables
  • .add_..._colors functions are all refactored into one .add_xstring_colors
  • update_..._palette functions are all refactored into one .update_X_palette function

@hpages
Copy link
Contributor

hpages commented Feb 21, 2025

Thanks for these changes. Coloring code is in good shape and is well documented. The unit tests look solid. Great work!

@hpages hpages merged commit ebea59e into Bioconductor:devel Feb 21, 2025
1 check passed
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.

2 participants