[REVIEW]: Backtest Graphics about joss-reviews HOT 91 CLOSED

✋ I can review this.

from joss-reviews.

Sorry for the inconvenience @desilinguist. I'm fairly certain that this is a problem in your local machine since simple dygraph plots don't show up. However, I understand your predicament in having to delete and reinstall everything. Thanks for your help in the process.

from joss-reviews.

https://github.com/joshuaulrich is an experienced R developer in financial time-series and has volunteered to review for us before.

from joss-reviews.

@arfon I'd be happy to review. I remember this package from R/Finance 2016.

from joss-reviews.

Sorry for the delay @arfon. Started a new job on 10/9 and added child number 3 on 11/1. It's been a busy couple of months.

@karantibrewal Here are some minor comments on the PDF:

• p2 ¶1 Unnecessary space between "futures" and comma (", futures , and")
• p2 ¶2 s/Quantopia/Quantopian/
• p2 ¶4 The abbreviation NMV is not previously defined
• p2 ¶4 Reference the figure number instead of "Below is a screen shot of the plots". E.g., "Figure 2 contains a screenshot of the plots"
• p2 ¶5 s/backtestgraphics/backtestGraphics/

And some comments on the package:

The GitHub version (0.1.5) is still behind the CRAN version (0.1.6). Ideally, the Version and Date fields in the DESCRIPTION file should be incremented.

I initially thought the Shiny interface was not working because no data are displayed when it opens. It would be helpful if the initial Shiny page contained instructions or a note that you have to click the [Visualize] button before you will see any results.

The "Strategies" drop down input contains the 3 strategies in commodity$strategy, but also contains 7 other strategies (e.g. Strategy 1.1). It's not clear what those are. You could use the "optgroup" functionality of selectInput to label the portfolio-level sub-strategies.

The previous comment also applies to the "Instruments" drop down input.

You should try to use synchronization to link the zooming of the plots.

It's very difficult to interpret the P&L alongside the market value for any aggregate because they can simultaneously move in opposite directions if a product is added/removed from the aggregate. For example, look at the livestock aggregate for strategy 1 and portfolio 1 on in 2005-11-22. The screenshot below shows a positive P&L of ~13.5k while the gross market value falls by ~3.5m. This is because substrategy 1.1 has observations on 2005-11-21 but not on 2005-11-22 and the market value of substrategy 1.1 is not carried forward to days it does not have P&L.

The GitHub repository does not contain instructions for how to contribute to the project or seek help. You can look at the quantmod contributing guide as an example. It contains several references to other contributing guides. You might also consider adding an issue and/or pull request template. I have an issue template for quantmod.

from joss-reviews.

Will review this weekend. I did notice that backtestGraphics was removed from CRAN since late January because R CMD check problems were not addressed. I ran R CMD check locally and on the default platforms for rhub::check_for_cran(), but I did not see any obvious issues. @karantibrewal, what issue(s) did CRAN ask to be fixed?

from joss-reviews.

@karantibrewal - thanks for submitting this. I have a couple of questions/comments before we can proceed with this submission:

• Please update your paper.md metadata. It still has me listed as an author
• Isn't not obvious to me that this software has a research application. Could you please explain what areas of research this software is designed to serve?

from joss-reviews.

Hi Arfon,

Thank you for your email. To address your points:

1. Sorry about that. I've made the changes, and also included a paper.json
   file.
2. Although our package doesn't solve a research question directly, we have
   designed it to help as a tool for others in visualization of financial
   data. Recently, we presented the package to the RFinance Conference, and
   received great feedback on how our package can be directly used by several
   in their research. Another advantage is that the package can be modified to
   support visualization of any time series data (doesn't have to be finance),
   according to the user's preference.

I hope that answers your question. Please let me know if you need anything
else.

Thanks again,
Karan

On Mon, Jun 20, 2016 at 7:23 PM, Arfon Smith [email protected]
wrote:

@karantibrewal https://github.com/karantibrewal - thanks for submitting
this. I have a couple of questions/comments before we can proceed with this
submission:

• Please update your paper.md
  https://github.com/knightsay/backtestGraphics/blob/master/paper.md
  metadata. It still has me listed as an author
• Isn't not obvious to me that this software has a research
  application. Could you please explain what areas of research this software
  is designed to serve?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#30 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AJ9Q9oS3kxwKytY0LQHO-C4ziZ71WmAsks5qNyD2gaJpZM4I6ONu
.

from joss-reviews.

OK thanks for the background @karantibrewal. It looks like the paper metadata is still a little wonky. It needs to be formatted as follows:

---
title: 'Fidgit: An ungodly union of GitHub and figshare'
tags:
  - example
  - tags
  - for the paper
authors:
 - name: Arfon M Smith
   orcid: 0000-0002-3957-2474
   affiliation: GitHub Inc.
date: 14 February 2016
bibliography: paper.bib
---

i.e. each author needs a name, orcid and affiliation field.

from joss-reviews.

from joss-reviews.

/ cc @openjournals/joss-reviewers - would anyone be willing to review this submission?

If you would like to review this submission then please comment on this thread so that others know you're doing a review (so as not to duplicate effort). Something as simple as :hand: I am reviewing this will suffice.

Reviewer instructions

• Please work through the checklist at the start of this issue.
• If you need any further guidance/clarification take a look at the reviewer guidelines here http://joss.theoj.org/about#reviewer_guidelines
• Please make a publication recommendation at the end of your review

Any questions, please ask for help by commenting on this issue! 🚀

from joss-reviews.

Before I start my review, I have a somewhat basic question. I am wondering why the name of the author making the submission ("Karan Tibrewal") does not appear in a bunch of the documentation including the main README and the R vignette etc. That should really be fixed.

Now on to my review:

1. The name of the license file is misspelled (LISENCE instead of LICENSE).

2. The release version says 0.1.5 and not 1.0.0.

3. There are no installation instructions in the main README. However, there seem to be installation instructions in the .Rmd file that work just fine. These should also be added to the main README.

4. I looked at the documentation via help(backtestGraphics) and it seems to be fairly complete. However, there seems to be some weird formatting in that file at the end:

   Examples:
   
        ## Not run:
   
        backtestGraphics(data = credit.bt)
        ## End(Not run)
   

5. There are no performance claims made in the documentation so I couldn't verify them.

6. Since I don't really know that much about the financial domain, once I installed the package, it was hard for me to figure out how to try it based just on the README. However, once I ran vignette('backtestGraphics'), I got a really nice PDF that told me that there was sample data available and how I can try out backtestGraphics on that data.

7. I ran the first command suggested by the vignette backtestGraphics(x = commodity), it launched the local shiny app but I couldn't get any plots to show up when I clicked on the Visualize button. However, the online shiny app linked to from the README does produce the graphs. So, perhaps something is broken locally or may be I am doing something wrong? I tried on both Safari and Chrome on my Mac with OS X 10.11.3.

8. Another thing that I noticed was that although a bunch of dependencies were installed when I first installed backtestGraphics, it prompted me to update shiny at the time of running the vignette example. It should really enforce that dependency at installation time.

9. There seem to be tests available but they don't seem to be run automatically and there is no documentation for how to run them.

10. I just noticed that paper.md still has some incorrect formatting at the top.

11. Although @karantibrewal says that this can be used for any time series data, the documentation is very, very specific to financial data and there's no documentation on how one would use this for non-financial data.

In conclusion, I think this is a niche shiny app that seems to be very focused on visualizing financial time series data. As it stands, I am not 100% sure that this has significant application in research but it can perhaps be a useful visualization tool for some researchers. I really think that the main README needs to have a lot more information and the statement of need really needs to be strengthened especially if the authors are claiming that the tool can visualize data outside of the financial domain.

from joss-reviews.

@desilinguist thanks a lot for the feedback. Sorry about the loose ends. We are working on it, and will address all of the aforementioned points by the end of this week.

from joss-reviews.

It is important that it is clear what the scientific contribution is of this work. The authors can give some examples of financial research application, preferably with figures in the docs/vignette. It would be even better if the authors can think of a broader application in scientific research and expand their audience.

from joss-reviews.

@whedon assign @desilinguist as reviewer

from joss-reviews.

OK, the reviewer is @desilinguist

from joss-reviews.

@desilinguist thanks a lot for the feedback. Sorry about the loose ends. We are working on it, and will address all of the aforementioned points by the end of this week.

@karantibrewal - could we have a status update on this submission please? Are you still working on the feedback from @desilinguist?

from joss-reviews.

@desilinguist @arfon Sorry for the delay! My team has had some back and
forth with the feedback. We have decided on the following: Backtest Graphics
is not a package that explicitly forwards a topic of research, but instead,
it is a tool that researchers can use to visualize backtest data.

We think our tool could be very relevant for researchers. Financial data is
almost always presented as non-interactive graphs or tables which are
difficult to decipher. Our package gives a quick and efficient avenue to
improve on this (for examples of working papers that could leverage our
work, see here
http://poseidon01.ssrn.com/delivery.php?ID=131074121084086071092103006092110093109023030014084091014027003126031066108084004090035056097013011097115069069031019089020121027020011005061088103084114104003080093061020081065105082020113117021025126103113002025085004089093106079081004015089095090073&EXT=pdf
and here
http://poseidon01.ssrn.com/delivery.php?ID=479004100026072008087021121123114090056069085080028027023073016081091004007096069105004042026057051007117120107082111114008040017035051049100079028086115107065002003044091016065102072117075006087096100113070123109003125096100117001011023126118103&EXT=pdf
).

Please let me know if you have any other questions. We are excited to
contribute to the open source community, and would love to help out in any
way that we can.

On Sat, Oct 8, 2016 at 9:34 AM, Arfon Smith [email protected]
wrote:

@desilinguist https://github.com/desilinguist thanks a lot for the
feedback. Sorry about the loose ends. We are working on it, and will
address all of the aforementioned points by the end of this week.

@karantibrewal https://github.com/karantibrewal - could we have a
status update on this submission please? Are you still working on the
feedback from @desilinguist https://github.com/desilinguist?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#30 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AJ9Q9usIeMvwmKoJ9Rj_0_Rlw0jvQ4lWks5qx5vQgaJpZM4I6ONu
.

from joss-reviews.

@desilinguist @arfon

My co-author just explained to me that I should be more clear in my explanation. Let me try again. I've addressed @desilinguist's points below:

The name of the license file is misspelled (LISENCE instead of LICENSE).

Fixed.

The release version says 0.1.5 and not 1.0.0.

Fixed.

There are no installation instructions in the main README. However, there seem to be installation instructions in the .Rmd file that work just fine. These should also be added to the main README.

Fixed.

Since I don't really know that much about the financial domain, once I installed the package, it was hard for me to figure out how to try it based just on the README. However, once I ran vignette('backtestGraphics'), I got a really nice PDF that told me that there was sample data available and how I can try out backtestGraphics on that data.

Added instructions to the README.

I ran the first command suggested by the vignette backtestGraphics(x = commodity), it launched the local shiny app but I couldn't get any plots to show up when I clicked on the Visualize button. However, the online shiny app linked to from the README does produce the graphs. So, perhaps something is broken locally or may be I am doing something wrong? I tried on both Safari and Chrome on my Mac with OS X 10.11.3.

Apologies! Do you think you can provide me with more details on this? I've tried the package on several computers and it seems to be working fine. If you can give me more information/ screenshots, I can look into this further.

I just noticed that paper.md still has some incorrect formatting at the top.

Fixed.

Although @karantibrewal says that this can be used for any time series data, the documentation is very, very specific to financial data and there's no documentation on how one would use this for non-financial data.

Apologies for the miscommunication! This is a package for financial data, and specifically, for backtest results only.

I think this is a niche shiny app that seems to be very focused on visualizing financial time series data. As it stands, I am not 100% sure that this has significant application in research but it can perhaps be a useful visualization tool for some researchers.

We agree! However, we believe that it is a fairly large niche. For example, we found several working papers on SSRN that could benefit from our package. Most of these papers present their data as tables or static graphics. Our package aims to enable these researchers to provide informative and interactive graphics, without going through the trouble of creating them themselves.

I really think that the main README needs to have a lot more information and the statement of need really needs to be strengthened especially if the authors are claiming that the tool can visualize data outside of the financial domain.

Our package only visualizes financial, and more specifically, backtest data. I have updated the README to make this more explicit.

@desilinguist @arfon Hope this was helpful. Looking forward to any other feedback you may have!

from joss-reviews.

@desilinguist - do you think you could take another look at this submission? @karantibrewal has responded to much of your feedback.

from joss-reviews.

@karantibrewal, thanks for making all the changes. I think they help make it clear exactly what the package does and the exact scenarios under which it can be useful to researchers studying financial data. One suggestion, it would be nice to explain in the README that to get more detailed documentation and background, users should run vignette("backtestGraphics") for people who may be new to R.

More importantly, I am still not able to get the local shiny app to actually show me any plots. I basically followed the commodity example in the README and ran the two commands in RStudio and got an error after hitting the "Visualize" button. I am attaching the screenshot here.

from joss-reviews.

@desilinguist thank you for your feedback. Ill go over the issues you
outlined, and revert by begining of next week.

Appreciate the help!

On Oct 24, 2016 9:21 AM, "Nitin Madnani" [email protected] wrote:

@karantibrewal https://github.com/karantibrewal, thanks for making all
the changes. I think they help make it clear exactly what the package does
and the exact scenarios under which it can he useful to researchers
studying financial data. One suggestion, it would be nice to explain in the
README that to get more detailed documentation and background, users should
run vignette("backtestGraphics") for people who may be new to R.

More importantly, I am still not able to get the local shiny app to
actually show me any plots. I basically followed the commodity example in
the README and ran the two commands in RStudio and got an error after
hitting the "Visualize" button. I am attaching the screenshot here.

[image: screen shot 2016-10-24 at 9 18 54 am]
https://cloud.githubusercontent.com/assets/161680/19647254/3866993e-99cb-11e6-9b31-c48df128c59e.png

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#30 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AJ9Q9jYbhL8plgFDuebrD4aovMiu6U1fks5q3LDqgaJpZM4I6ONu
.

from joss-reviews.

@desilinguist We've been working on trying to fix your issue. Even though the error message still comes up, you should be able to visualize the graphs now. Can you please confirm? It works on my end:

from joss-reviews.

I removed the package and re-installed it and then ran backtestGraphics(x = commodity) again in RStudio. But it still doesn't work for me. No plots and same error.

from joss-reviews.

@desilinguist thanks for your prompt response. aah that’s so weird. just to
be sure, are you using a mac book?

On Nov 4, 2016, at 9:14 AM, Nitin Madnani [email protected] wrote:

I removed the package and re-installed it and then ran backtestGraphics(x =
commodity) again in RStudio. But it still doesn't work for me. No plots and
same error.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#30 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AJ9Q9sh8oWX_l2N04CwZ0qxDvvcJwOFCks5q6y-wgaJpZM4I6ONu
.

from joss-reviews.

Yes, I am using a MacBook Pro running macOS 10.12.1 (R version 3.3.0, RStudio version 0.99.903).

from joss-reviews.

Hi @desilinguist, seems to be a dependency issue. We'll fix it for the package, but can you please confirm before that?

devtools::install_version("shiny", version="0.13.2",repos = "http://cran.us.r-project.org")
install.packages("backtestGraphics")
backtestGraphics(x = commodity)

This should work. Please let me know.

from joss-reviews.

Thanks! I'm currently on vacation until early December but will take a look once I am back.

from joss-reviews.

Thanks, Nitin. Safe travels.

On Nov 18, 2016 7:34 PM, "Nitin Madnani" [email protected] wrote:

Thanks! I'm currently on vacation until early December but will take a
look once I am back.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#30 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AJ9Q9s1A792fSZ2zOwHkYlQbxd6wwu6oks5q_kQFgaJpZM4I6ONu
.

from joss-reviews.

from joss-reviews.

I followed your provided instructions and I still don't see the graphs although I do see the numbers on the left now.

from joss-reviews.

from joss-reviews.

Friendly new year bump on this @desilinguist 😁

from joss-reviews.

Thanks @arfon!

@karantibrewal, sorry for the delay. Here are my package versions:

from joss-reviews.

from joss-reviews.

Hi @desilinguist,
I've tried recreating your bug in the past month on a bunch of computers but with little success -- however, I'm eager to work with you to fix this, and I appreciate your patience.

One possible reason that the package might be failing is if "dygraph" isn't set up properly on your computer. The following code snippet will test that:

library(backtestGraphics)
library(dygraphs)
library(dplyr)
library(xts)

sum_data <- function(x, sector.selected = NULL, capital.num = NULL){

sector <- nmv <- pnl <- contract <- gmv <- NULL

if(!is.null(sector.selected)) {
x <- filter(x, sector == sector.selected)
}

if(dim(x)[1] == 0){
stop("There does not exist any data that satisfies your selection.
Please select a new combination of Strategy, Portfolio and Instrument.")
}

x <- group_by(x, date)

x.sum <- summarise(x,
nmv = sum(nmv, na.rm = TRUE),
pnl = sum(pnl, na.rm = TRUE),
contract = sum(contract, na.rm=TRUE),
gmv = sum(gmv, na.rm = TRUE))
x.sum <- tbl_df(x.sum) %>%
arrange(date) %>%
filter(gmv != 0)

if(is.null(capital.num)){
x.sum <- mutate(x.sum, ret = pnl / gmv, cumpnl = cumsum(pnl))
} else {
x.sum <- mutate(x.sum, ret = pnl / capital.num, cumpnl = cumsum(pnl))
}

x.sum <- mutate(x.sum, name = " ")

if(!is.null(sector.selected)){
x.sum[[1,8]] <- sector.selected
} else {
x.sum[[1,8]] <- "the Whole Portfolio"
}

return(x.sum)

}

slice_data <- function(x, input, capital.num){

name <- id <- portfolio <- gmv <- nmv <- contract <- x.name <- strategy <- NULL
substrategy <- capital.num <- pnl <- sector <- NULL

x.name <- unique(select(x, name, id, sector, portfolio, strategy, substrategy))

f <- list()
f$x.temp <- x %>%
group_by(name, id, date, sector, portfolio) %>%
summarise(gmv = sum(gmv, na.rm = TRUE),
nmv = sum(nmv, na.rm = TRUE),
pnl = sum(pnl, na.rm = TRUE),
contract = sum(abs(contract), na.rm = TRUE)) %>%
ungroup()
f$x.temp <- f$x.temp %>%
group_by(name, id, date, sector) %>%
summarise(gmv = sum(gmv, na.rm = TRUE),
nmv = sum(nmv, na.rm = TRUE),
pnl = sum(pnl, na.rm = TRUE),
contract = sum(abs(contract), na.rm = TRUE)) %>%
ungroup()
f$x <- sum_data(f$x.temp,
capital.num = capital.num)

f$instrument <- length(unique(f$x.temp[["id"]]))
return(f)
}

cumpnl_plot <- function(x) {

date <- name <- cumpnl <- NULL

title.var <- paste("Cumulative P&L for", x$name[1])
x <- xts(x$cumpnl, order.by = x$date)
dygraph(x, main = title.var) %>%
dySeries(label = "Cumulative P&L") %>%
dyRangeSelector() %>%
dyLegend(labelsSeparateLines = TRUE) %>%
dyOptions(labelsKMB = TRUE,
useDataTimezone = TRUE,
fillGraph = TRUE)
}

data <- slice_data(commodity, NULL, NULL)
p1 <- cumpnl_plot(x = data$x)
p1

Please run the above on your R studio console. You should hopefully see a plot like:

Please let me know what you see. Thanks.

from joss-reviews.

Hi @desilinguist, gentle follow up on this - please let me know if the above works for you.

Thanks,
Karan

from joss-reviews.

When I run your code snippet, I get no errors but the graph is blank as shown below.

from joss-reviews.

Thanks @desilinguist. I believe there is a problem with the dygraphs package on your computer. I tried running the same commands with your version, but the graph was not blank. Could you try the following:

(1) Try reinstalling dygraphs, and running the previous code.
That is,

remove.packages("dygraphs")
install.packages("dygraphs")
library(backtestGraphics)
library(dygraphs)
library(dplyr)
library(xts)

sum_data <- function(x, sector.selected = NULL, capital.num = NULL){

sector <- nmv <- pnl <- contract <- gmv <- NULL

if(!is.null(sector.selected)) {
x <- filter(x, sector == sector.selected)
}

if(dim(x)[1] == 0){
stop("There does not exist any data that satisfies your selection.
Please select a new combination of Strategy, Portfolio and Instrument.")
}

x <- group_by(x, date)

x.sum <- summarise(x,
nmv = sum(nmv, na.rm = TRUE),
pnl = sum(pnl, na.rm = TRUE),
contract = sum(contract, na.rm=TRUE),
gmv = sum(gmv, na.rm = TRUE))
x.sum <- tbl_df(x.sum) %>%
arrange(date) %>%
filter(gmv != 0)

if(is.null(capital.num)){
x.sum <- mutate(x.sum, ret = pnl / gmv, cumpnl = cumsum(pnl))
} else {
x.sum <- mutate(x.sum, ret = pnl / capital.num, cumpnl = cumsum(pnl))
}

x.sum <- mutate(x.sum, name = " ")

if(!is.null(sector.selected)){
x.sum[[1,8]] <- sector.selected
} else {
x.sum[[1,8]] <- "the Whole Portfolio"
}

return(x.sum)

}

slice_data <- function(x, input, capital.num){

name <- id <- portfolio <- gmv <- nmv <- contract <- x.name <- strategy <- NULL
substrategy <- capital.num <- pnl <- sector <- NULL

x.name <- unique(select(x, name, id, sector, portfolio, strategy, substrategy))

f <- list()
f$x.temp <- x %>%
group_by(name, id, date, sector, portfolio) %>%
summarise(gmv = sum(gmv, na.rm = TRUE),
nmv = sum(nmv, na.rm = TRUE),
pnl = sum(pnl, na.rm = TRUE),
contract = sum(abs(contract), na.rm = TRUE)) %>%
ungroup()
f$x.temp <- f$x.temp %>%
group_by(name, id, date, sector) %>%
summarise(gmv = sum(gmv, na.rm = TRUE),
nmv = sum(nmv, na.rm = TRUE),
pnl = sum(pnl, na.rm = TRUE),
contract = sum(abs(contract), na.rm = TRUE)) %>%
ungroup()
f$x <- sum_data(f$x.temp,
capital.num = capital.num)

f$instrument <- length(unique(f$x.temp[["id"]]))
return(f)
}

cumpnl_plot <- function(x) {

date <- name <- cumpnl <- NULL

title.var <- paste("Cumulative P&L for", x$name[1])
x <- xts(x$cumpnl, order.by = x$date)
dygraph(x, main = title.var) %>%
dySeries(label = "Cumulative P&L") %>%
dyRangeSelector() %>%
dyLegend(labelsSeparateLines = TRUE) %>%
dyOptions(labelsKMB = TRUE,
useDataTimezone = TRUE,
fillGraph = TRUE)
}

data <- slice_data(commodity, NULL, NULL)
p1 <- cumpnl_plot(x = data$x)
p1

(2) If you still see a blank plot, try a simpler plot:

library(dygraphs)
library(xts)
df <- data.frame(x = 1:10, y = as.Date(seq(as.Date("2017/1/1"), as.Date("2017/1/10"), "days")))
x <- xts(df$x, order.by = df$y)
dygraph(x)

Please let me know what you see. Thank you, again, for your time!

from joss-reviews.

I still see a blank graph after removing and re-installing dygraphs, for both snippets.

from joss-reviews.

Thanks for your prompt reply @desilinguist! This is definitely an issue with the Dygraphs package.

Could you try running the "simple plot" code snippet on the terminal in R rather than RStudio? Do you see the graph on your browser now?

Another suggestion is that you try to delete and reinstall R, RStudio, and try running the simple plot again:

install.packages(c("dygraphs", "xts"))
library(dygraphs)
library(xts)
df <- data.frame(x = 1:10, y = as.Date(seq(as.Date("2017/1/1"), as.Date("2017/1/10"), "days")))
x <- xts(df$x, order.by = df$y)
dygraph(x)

from joss-reviews.

Even if I run on the terminal in R, I still get a blank graph.

@arfon Perhaps this should be assigned to another reviewer? I don't really want to delete and reinstall everything just for this one review.

from joss-reviews.

@arfon Perhaps this should be assigned to another reviewer? I don't really want to delete and reinstall everything just for this one review.

👍 ok no problem @desilinguist - thanks for all of your help thus far.

@karantibrewal - do you have any suggestions for alternative reviewers for this?

from joss-reviews.

from joss-reviews.

Hi @arfon! I believe the issue was with dygraphs (a graphing package in R). Otherwise, the package builds successfully. I could provide any further details required by your team.

OK thanks. It's not clear to me whether you're suggesting here that we don't need a followup review of this? Could you advise @karantibrewal & @desilinguist?

from joss-reviews.

from joss-reviews.

I think another review will be good.

OK thanks for confirming @desilinguist.

@karantibrewal - do you have any suggestions for another reviewer?

from joss-reviews.

Sorry @arfon, I think I might have misunderstood you!

I agree that we require another review. Do you mean suggestions for people who could be reviewers? I don't have anyone at the top of my mind, but I could ask around in my network if people are willing. Are there any guidelines for selecting these reviewers (I see the guidelines for reviewing; what I mean is that is there any minimum qualification/requirements to sign up as a reviewer)?

from joss-reviews.

Do you mean suggestions for people who could be reviewers? I don't have anyone at the top of my mind, but I could ask around in my network if people are willing.

Yes, that's what I mean.

Are there any guidelines for selecting these reviewers (I see the guidelines for reviewing; what I mean is that is there any minimum qualification/requirements to sign up as a reviewer)?

Not really, other than that they should be experienced in the topic of the submission and the technologies used.

from joss-reviews.

from joss-reviews.

@arfon sorry, I'm not sure I can think of a suitable reviewer.

Just to be clear -- I'm an undergraduate. Should I ask a classmate?

from joss-reviews.

@arfon sorry, I'm not sure I can think of a suitable reviewer.

OK, no problem. @karthik - any chance you could assist here finding an additional reviewer?

from joss-reviews.

Hi @arfon and @karthik. Just a gentle follow up: is there any update on this? Thanks.

from joss-reviews.

@whedon list reviewers

from joss-reviews.

Here's the current list of JOSS reviewers: https://github.com/openjournals/joss/blob/master/docs/reviewers.csv

from joss-reviews.

👋 @cdcrabtree - would you be willing to review this submission for JOSS?

from joss-reviews.

hey @arfon, gentle follow up: could you please give us an update? Thanks.

from joss-reviews.

👋 @nuest - would you be willing to review this submission for JOSS?

from joss-reviews.

hey @arfon, gentle follow up: could you please give us an update? Thanks.

Still looking for a new reviewer sorry.

from joss-reviews.

No problem, we understand completely. Thanks for your help.

from joss-reviews.

@openjournals/ropensci - any chance you could help me find a second reviewer here?

from joss-reviews.

@joshuaulrich - would you be interested in reviewing this submission for JOSS?

from joss-reviews.

Alternatively the creator of https://github.com/EdwinTh/padr who works in finance (has mentioned interest in reviewing for rOpenSci )

from joss-reviews.

@whedon assign @joshuaulrich as reviewer

from joss-reviews.

OK, the reviewer is @joshuaulrich

from joss-reviews.

@arfon I'd be happy to review. I remember this package from R/Finance 2016.

@joshuaulrich - many thanks! Please update the checklist above and giving feedback in this issue. The reviewer guidelines are available here: http://joss.theoj.org/about#reviewer_guidelines

Any questions/concerns please let me know.

from joss-reviews.

Friendly reminder @joshuaulrich 😁

from joss-reviews.

@arfon Thank you! I forgot to add this to my to-do list and had forgotten about it. :(

from joss-reviews.

@joshuaulrich gentle follow up on this

from joss-reviews.

@karantibrewal Very strange that the GitHub version is behind the CRAN version. It appears the CRAN version has changes to R/backtestGraphics.R that are not in the GitHub repository.

from joss-reviews.

@karantibrewal I'm unable to continue reviewing until you tell me which version of the package I should look at (CRAN or GitHub). If the GitHub version is the latest, please fix the issue that prevents the package from building.

from joss-reviews.

@joshuaulrich sorry for delay. GitHub version is latest -- issues have been fixed now. Thanks for flagging.

from joss-reviews.

@joshuaulrich - over to you I think for another look :-)

from joss-reviews.

@joshuaulrich - over to you I think for another look :-)

👋 @joshuaulrich - friendly reminder to take another look at this sometime soon please.

from joss-reviews.

@karantibrewal - 👋 - did you get a chance to review @joshuaulrich's feedback yet?

from joss-reviews.

Thanks for the feedback @joshuaulrich! We will turn these in soon (some delay b/c of finals period).

from joss-reviews.

Ping @karantibrewal - are you still interested in pursuing this publication with JOSS?

from joss-reviews.

Oops - sorry about this @arfon

@joshuaulrich comments have been fixed now. Details are below:

p2 ¶1 Unnecessary space between "futures" and comma (", futures , and")
p2 ¶2 s/Quantopia/Quantopian/
p2 ¶4 The abbreviation NMV is not previously defined
p2 ¶4 Reference the figure number instead of "Below is a screen shot of the plots". E.g., "Figure 2 contains a screenshot of the plots"
p2 ¶5 s/backtestgraphics/backtestGraphics/

[Fixed. Although not quite sure what the last point means. I think we need to use the name of the package instead of saying "our package". I changed that.]

The GitHub version (0.1.5) is still behind the CRAN version (0.1.6). Ideally, the Version and Date fields in the DESCRIPTION file should be incremented.

Fixed

I initially thought the Shiny interface was not working because no data are displayed when it opens. It would be helpful if the initial Shiny page contained instructions or a note that you have to click the [Visualize] button before you will see any results.

Fixed.

The "Strategies" drop down input contains the 3 strategies in commodity$strategy, but also contains 7 other strategies (e.g. Strategy 1.1). It's not clear what those are. You could use the "optgroup" functionality of selectInput to label the portfolio-level sub-strategies.

The previous comment also applies to the "Instruments" drop down input.

Drop downs look much more informative now - thanks for this one!

You should try to use synchronization to link the zooming of the plots.

Fixed.

It's very difficult to interpret the P&L alongside the market value for any aggregate because they can simultaneously move in opposite directions if a product is added/removed from the aggregate. For example, look at the livestock aggregate for strategy 1 and portfolio 1 on in 2005-11-22. The screenshot below shows a positive P&L of ~13.5k while the gross market value falls by ~3.5m. This is because substrategy 1.1 has observations on 2005-11-21 but not on 2005-11-22 and the market value of substrategy 1.1 is not carried forward to days it does not have P&L.

This scenario can very well happen, and as I will explain now, it is not unreasonable.

Market value is a stock and P&L is a flow.

A stock is something that you measure at a specific moment in time. So, the market value of our cattle futures position is, roughly, number of contracts at a point in time multiplied by price per contract. Since the data we provide at a daily frequency, we only get a market value once per day.

P&L, on the other hand, is a flow - so it takes into account each trade you make. So for example, say you own 3 futures contracts today worth $ 1 mil. Tomorrow, you make day trades where you lose $50k. On close of market, you might still own 3 contracts worth $1 mil. In this case, your PL will be -50K but there will be no change to market value.

from joss-reviews.

@joshuaulrich @arfon gentle follow up on this. Please let us know your thoughts, and if there is anything else you'd like us to fix!

from joss-reviews.

Friendly reminder to take another look at this @joshuaulrich when you get a chance 😄

from joss-reviews.

from joss-reviews.

Oops, I did not respond to questions about my initial comments.

Although not quite sure what [p2 ¶5 s/backtestgraphics/backtestGraphics/] means.

The "s/foo/bar/" means substitute "foo" with "bar". It's a command you can do in many text programming languages (e.g. sed, awk) and is shorthand many programmers use. Sorry for the confusion! In this case, it means you need to change "backtestgraphics" with "backtestGraphics". The specific text is, "...more complex backtests, \pkg{backtestgraphics} allows the user...".

This scenario can very well happen, and as I will explain now, it is not unreasonable.

Right, I understand why it happens and I agree that the results are reasonable. My point is that how they are displayed makes it hard to interpret the aggregate results. I don't necessarily think this is a blocker to publication, but I do think it will cause confusion. I would try plotting aggregates as stacked bar charts of their components, and/or annotating the new bar with the current instrument count whenever it is not the same as the prior bar's instrument count.

New comments:

• In the summary table, "Allocated Capital" should be changed to "Allocated Capital (AC)". That will provide context for the "Annualized Return on AC (%)" and "Annualized Volatility on AC (%)" rows.
• Still need a contributing guide.
• With paper.md:
  • ¶1 has an unnecessary space between "futures" and comma (", futures , and")
  • ¶1 "default" is incorrectly spelled "defualt"
  • ¶5 accommodate"" is incorrectly spelled "accomodate"

Conclusion: My opinion is that this is ready for publication after the minor copy edits are done, the contributing guide is added to the GitHub repo, the example runs, and the package is back on CRAN.

Things to address for CRAN:

• Need to add .bib, .json, and paper.md to .Rbuildignore. Or put them in a new directory and add the directory name to .Rbuildignore. Whatever you choose, just make sure they're not in the package tarball you submit to CRAN (R CMD check --as-cran will warn you about them).
• Add LICENSE to DESCRIPTION (again, R CMD check --as-cran will warn about this).

from joss-reviews.

👍 thanks @joshuaulrich

from joss-reviews.

Thanks for your comments @joshuaulrich. They have been fixed now.

However, I can't get the CRAN issue fixed for the life of me. It seems like a file path issue. The error message specifically is:

Messages: sh: /usr/local/opt/texinfo/bin/texi2dvi: No such file or directory Calls: <Anonymous> -> texi2pdf -> texi2dvi Execution halted

Do you have any idea of how to fix this? Any advice would be much appreciated!

from joss-reviews.

@karantibrewal I can't see any of your fixes. In fact, I don't see any commits in the repo since 3/19. The error looks like you don't have texi2dvi installed. On Ubuntu, that's in the texinfo package.

from joss-reviews.

@joshuaulrich Sorry, what do you mean? All your comments from your previous post have been turned in (except for CRAN issue). These fixes were part of the 3/19 commit.

from joss-reviews.

@karantibrewal I was confused because you said the issues were fixed, but the last activity was a month ago; and the issues I opened for them had not been closed. It's good practice to put "Fixes #XYZ" in the commit that fixes an issue, so you have a cross-reference of the commit and issue. Also, GitHub (and GitLab, Bitbucket, etc) will automatically close issue #XYZ for you.

And DESCRIPTION is still in your .Rbuildignore. I commented about 2 weeks ago that this would mean the package cannot be installed. In fact, R CMD build backtestGraphics fails because the package can't be installed to build the vignettes.

from joss-reviews.

@karantibrewal - please revisit this review when you get a chance. It would be good to get this closed out.

from joss-reviews.

Ping @karantibrewal - please revisit this soon.

from joss-reviews.

After numerous attempts to contact the reviewer, I'm assuming @karantibrewal is no longer interested in pursuing this submission in JOSS.

@joshuaulrich - thank you very much for your time, and I'm sorry this didn't make it through review.

from joss-reviews.

from joss-reviews.