-
-
Notifications
You must be signed in to change notification settings - Fork 36
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
[REVIEW]: Backtest Graphics #30
Comments
@karantibrewal - thanks for submitting this. I have a couple of questions/comments before we can proceed with this submission:
|
Hi Arfon, Thank you for your email. To address your points:
I hope that answers your question. Please let me know if you need anything Thanks again, On Mon, Jun 20, 2016 at 7:23 PM, Arfon Smith notifications@github.com
|
OK thanks for the background @karantibrewal. It looks like the paper metadata is still a little wonky. It needs to be formatted as follows:
i.e. each author needs a name, orcid and affiliation field. |
Hi Arfon,
I've made the aforementioned changes.
Thanks,
Karan
|
/ 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 Reviewer instructions
Any questions, please ask for help by commenting on this issue! 🚀 |
✋ I can review this. |
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:
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. |
@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. |
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. |
@whedon assign @desilinguist as reviewer |
OK, the reviewer is @desilinguist |
@karantibrewal - could we have a status update on this submission please? Are you still working on the feedback from @desilinguist? |
@desilinguist @arfon Sorry for the delay! My team has had some back and We think our tool could be very relevant for researchers. Financial data is Please let me know if you have any other questions. We are excited to On Sat, Oct 8, 2016 at 9:34 AM, Arfon Smith notifications@github.com
|
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:
Fixed.
Fixed.
Fixed.
Added instructions to the README.
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.
Fixed.
Apologies for the miscommunication! This is a package for financial data, and specifically, for backtest results only.
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.
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! |
@desilinguist - do you think you could take another look at this submission? @karantibrewal has responded to much of your feedback. |
@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 More importantly, I am still not able to get the local shiny app to actually show me any plots. I basically followed the |
@desilinguist thank you for your feedback. Ill go over the issues you Appreciate the help! On Oct 24, 2016 9:21 AM, "Nitin Madnani" notifications@github.com wrote:
|
@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: |
I removed the package and re-installed it and then ran |
@desilinguist thanks for your prompt response. aah that’s so weird. just to On Nov 4, 2016, at 9:14 AM, Nitin Madnani notifications@github.com wrote: I removed the package and re-installed it and then ran backtestGraphics(x = — |
Yes, I am using a MacBook Pro running macOS 10.12.1 (R version 3.3.0, RStudio version 0.99.903). |
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") This should work. Please let me know. |
Thanks! I'm currently on vacation until early December but will take a look once I am back. |
Thanks, Nitin. Safe travels. On Nov 18, 2016 7:34 PM, "Nitin Madnani" notifications@github.com wrote:
|
@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. |
@joshuaulrich sorry for delay. GitHub version is latest -- issues have been fixed now. Thanks for flagging. |
@joshuaulrich - over to you I think for another look :-) |
👋 @joshuaulrich - friendly reminder to take another look at this sometime soon please. |
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:
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 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. |
@karantibrewal - 👋 - did you get a chance to review @joshuaulrich's feedback yet? |
Thanks for the feedback @joshuaulrich! We will turn these in soon (some delay b/c of finals period). |
Ping @karantibrewal - are you still interested in pursuing this publication with JOSS? |
Oops - sorry about this @arfon @joshuaulrich comments have been fixed now. Details are below:
[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.]
Fixed
Fixed.
Drop downs look much more informative now - thanks for this one!
Fixed.
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. |
@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! |
Friendly reminder to take another look at this @joshuaulrich when you get a chance 😄 |
Will review this weekend. I did notice that backtestGraphics was removed from CRAN since late January because |
D'uh! Missed that notice because we failed to maintain a current maintainer
e-mail address. So, not sure what CRAN wanted fixed.
Karan: Please make yourself the maintainer and resubmit to CRAN.
…On Sat, Mar 17, 2018 at 12:42 PM, Joshua Ulrich ***@***.***> wrote:
@afron <https://github.com/afron>, @karantibrewal
<https://github.com/karantibrewal>,
Will review this weekend. I did notice that backtestGraphics was removed
from CRAN
<https://cran.r-project.org/web/packages/backtestGraphics/index.html>
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
<https://github.com/karantibrewal>, what issue(s) did CRAN ask to be
fixed?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#30 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEV4k-Y5VraGm1os99HiMiO29-1Hlkbnks5tfQSkgaJpZM4I6ONu>
.
|
Oops, I did not respond to questions about my initial comments.
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...".
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:
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:
|
👍 thanks @joshuaulrich |
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:
Do you have any idea of how to fix this? Any advice would be much appreciated! |
@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 |
@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. |
@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 |
@karantibrewal - please revisit this review when you get a chance. It would be good to get this closed out. |
Ping @karantibrewal - please revisit this soon. |
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. |
No need to apologise. Let me know if there are other finance or time series
reviews I can help with.
|
Submitting author: @karantibrewal (Karan Tibrewal)
Repository: https://github.com/knightsay/backtestGraphics
Version: v1.0.0
Editor: @arfon
Reviewer: @joshuaulrich
Status
Status badge code:
Reviewer questions
Conflict of interest
General checks
Functionality
Documentation
Software paper
Paper PDF: 10.21105.joss.00030.pdf
paper.md
file include a list of authors with their affiliations?The text was updated successfully, but these errors were encountered: