Allow perTradeStats to accept a vector of symbols in the Symbol argument

braverock / blotter

blotter provides transaction infrastructure for defining transactions, portfolios and accounts for trading systems and simulation. Provides portfolio support for multi-asset class and multi-currency portfolios. Actively maintained and developed.

114 stars 50 forks source link

Allow perTradeStats to accept a vector of symbols in the Symbol argument #79

Closed vi-to closed 5 years ago

vi-to commented 5 years ago

This pull request adds the feature firstly requested in issue #42, as further discussed in #43 and detailed by @braverock in comments.

vi-to commented 5 years ago

If you prefer, you can close this pull request and I can open a new one with the intended code and its documentation only. Hope this may facilitate your reviews.

braverock commented 5 years ago

@vi-to you don't need to close this pull request, just clean it up so that it only contains the change you are proposing for this issue.

jaymon0703 commented 5 years ago

I spent some time reviewing the final commit today and the output seems sound. I have asked Ross, who raised the request over 2 years ago, for his opinion. Output below using rbind() and 2 symbols from the 'rsi' demo in quantstrat:

vi-to commented 5 years ago

@braverock I have tried to rebase the branch so that only the last commit was kept. Now, git status reports that my local branch and the remote one (iss42-43) from which this pull request came from have diverged, as expected. In particular, that the branch I am about to push has only one commit; my hope is that once pushed this will revert the unwanted commits only. However, I find this an unusual situation, in case it fails I would please you to give me further indications.

@jaymon0703 glad you are reviewing it and that it is working fine.

vi-to commented 5 years ago

So, I spent hours to fix this. As you can see I removed some old unrelated commits, but then retrieved many others that are absolutely unrelated with the topic; thus resulting in the end in an even bigger mess than it was. I want to fix this as soon as possible. These are the steps I followed:

git checkout my-branch, locally;
git rebase -i HEAD~n, picking only the needed commit 6d2c1eb. This gives me local conflicts with the two files in the commit, which seems unbelievable to me since these are the files I committed first, but that nonetheless I solve as suggest here and here;
git push --force to push after rebase.

Can you please explain what am I doing wrong?

Also, a few minutes ago I have seen a behavior I absolutely did not think was even possible (I don't have write permissions on this), that is of closing the pull request and merging it. What does this mean?

vi-to commented 5 years ago

I am still writing here to traceability purposes. Attempts to clean up this pull request failed. I think I was in the middle of a bad rebasing situation with a detached HEAD and local conflicts that prevented its correct use. However, relevant commits are backed up in a different new local branch. I feel I messed enough with this branch and the related pull request, therefore I am not going to touch it any longer since it is giving us more problems than it solves. It seems options are:

you prefer to reopen this pull request and want me to still work on this;
I delete this bad branch and start over, either from the local branch I mentioned above or from a totally new one;

If you choose 1), please provide me detailed information (it may be appropriate to undo the rebase and I have no experience on this). I would personally prefer 2) because it seems easier and cleaner, after all this pull request is somehow already closed. I am waiting for indications, thank you for your patience.

braverock commented 5 years ago

@vi-to go ahead and delte this branch and create a clean one.