-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add UI for user to toggle median line, y: raw values/ log2 values #41
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work overall - you've got a workable solution, just a few suggestions to perhaps clean up the code in the review here.
One more note - right now, I believe the "Show Median Line" box appears even on the Multiplot, which doesn't support showing a median line. Would be nice to have that only appear on plot types that support it.
server.R
Outdated
# Render log2/raw graph based on user input | ||
if (show.raw()){ | ||
g <- ggplot(clinical,aes(fill=Gene, | ||
y=2^Expression, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make the code simpler to scale the values before the plotting code. So instead of two plotting chunks, you would do something like:
if(show.log2){
clinical$Expression = log2(clinical$Expression)
}
then just one complicated ggplot
block below.
Is there a downside? (I think you're already keeping track of the state of the data with the show.raw/show.log2 param so you'll know when and which way to toggle, right?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line actually modifies the original data in the plot. St like this maybe better:
"""
if (show.log2) g <- ggplot(y=Expression)
else : g <- ggplot(y=2^expression)
g + big_data_stuff
"""
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure - that's eminently reasonable
I just pushed my update, can you have a look at it? @chrisamiller |
server.R
Outdated
y_label <- "Log2 Expression" | ||
} else { | ||
y_aes <- aes(y = 2^Expression) | ||
y_label <- "Raw Expression" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more minor change - instead of "Raw Expression", just use "Expression". (there are several steps before it gets to this plotting point that alter the values, normalize them, etc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one very minor change to the axis label. Otherwise, looks great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Some added features so that users can better customize their graphs