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

Add ylab to plot.xts #334

Merged
merged 3 commits into from May 10, 2020
Merged

Conversation

jaymon0703
Copy link
Contributor

@jaymon0703 jaymon0703 commented May 9, 2020

Few things worth mentioning:

  1. Tried to do this in chart.lines() but ylab was not passing through
  2. Need a solution for when multi.panel is not NULL, perhaps a character vector of ylab strings?
  3. Needed the plot printed before i could add title() hence cs becomes plot(cs)

Simple test is add ylab argument to plot.xts()

See #333

Please review the contributing guide before submitting your pull request. Please pay special attention to the pull request and commit message sections. Thanks for your contribution and interest in the project!

Few things worth mentioning:

1. Tried to do this in chart.lines() but ylab was not passing through
2. Need a solution for when multi.panel is not NULL, perhaps a character vector of ylab strings?
3. Needed the plot printed before i could add title() hence cs becomes plot(cs)

Simple test is add ylab argument to plot.xts()

See joshuaulrich#333
@jaymon0703
Copy link
Contributor Author

jaymon0703 commented May 9, 2020

I should mention i did try sub and xlab args, but they overlap the x-axis labels so would not work without some extra effort which im not sure is warranted.

Copy link
Owner

@joshuaulrich joshuaulrich left a comment

At minimum, the hasArg() bug needs to be fixed. We also should not force the plot to be rendered. That's what all the expression logic and chart.lines() accomplish.

R/plot.R Outdated Show resolved Hide resolved
R/plot.R Outdated Show resolved Hide resolved
@joshuaulrich
Copy link
Owner

joshuaulrich commented May 10, 2020

This seems to work for me. Can you try it and see if it does what you expect?

diff --git a/R/plot.R b/R/plot.R
index 01474a3..11b6fcc 100644
--- a/R/plot.R
+++ b/R/plot.R
@@ -286,6 +286,7 @@ plot.xts <- function(x,
   cs$Env$column_names <- colnames(x)
   cs$Env$nobs <- NROW(cs$Env$xdata)
   cs$Env$main <- main
+  cs$Env$ylab <- if (hasArg("ylab")) eval.parent(plot.call$ylab) else ""
   
   # Set xlim using the raw returns data passed into function
   # xlim can be based on observations or time
@@ -411,6 +412,10 @@ plot.xts <- function(x,
                              col=theme$labels, srt=theme$srt, offset=1, pos=4,
                              cex=theme$cex.axis, xpd=TRUE)))
   }
+
+  # ylab
+  exp <- c(exp, expression(title(ylab = ylab[1], mgp = c(1, 1, 0))))
+
   cs$add(exp, env=cs$Env, expr=TRUE)
   
   # add main series

@jaymon0703
Copy link
Contributor Author

jaymon0703 commented May 10, 2020

Thanks Josh. Is there a way for me to pull your changes, or must i add them manually?

@joshuaulrich
Copy link
Owner

joshuaulrich commented May 10, 2020

@jaymon0703
Copy link
Contributor Author

jaymon0703 commented May 10, 2020

I can push them to your branch if they look good to you.

Thanks Josh, it works. Feel free to push.

@joshuaulrich
Copy link
Owner

joshuaulrich commented May 10, 2020

I just noticed your comment about how to handle when multi.panel != NULL. Did you try that with my change? Do you think we should try adding that now, or wait to handle it in a separate issue/PR? My slight preference would be to merge this and deal with the multi.panel case later.

@jaymon0703
Copy link
Contributor Author

jaymon0703 commented May 10, 2020

Later is fine. I have pulled your changes, thanks. It seems the PR is updated with your changes. Is there anything left for me to do on the PR?

@joshuaulrich joshuaulrich merged commit 64d2cb8 into joshuaulrich:master May 10, 2020
1 check passed
@jaymon0703 jaymon0703 deleted the 333_add_ylab branch May 10, 2020
@joshuaulrich
Copy link
Owner

joshuaulrich commented May 10, 2020

Nothing more for you to do. I just needed to merge. Thanks for this suggestion and feedback!

@jaymon0703
Copy link
Contributor Author

jaymon0703 commented May 10, 2020

Thank you Josh.

@jaymon0703
Copy link
Contributor Author

jaymon0703 commented May 10, 2020

The idea came from some algoTCA charts we plot with xts in blotter. Now we can show the metric for the y-axis. Thanks again.
image

@joshuaulrich joshuaulrich added this to the 0.12-1 milestone Aug 2, 2020
@joshuaulrich joshuaulrich linked an issue Aug 2, 2020 that may be closed by this pull request
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.

No 'ylab' param in plot.xts
2 participants