search

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

Trading Price Performance measure + minor spelling error #88

Closed anshul96go closed 5 years ago

jaymon0703 commented 5 years ago

Thanks @anshul96go this PR seems to be submitted on the master branch, and we should be targeting the 54_algotca branch.

Nevertheless, i reviewed the function (even though for some reason i do not see your file changes when i run git pull https://github.com/anshul96go/blotter.git patch-2 but these are my thoughts:

  1. i think trdPrcPerf is a better name for the function
  2. We should probably store the output in a dataframe with some headers
  3. We could probably specify the metric "bps" in the output

This is close to being merge-ready. I may take some liberties to add to the documentation if you dont mind, and some of the structure to maintain consistency with other functions in blotter, as well as include an example.

Thanks!

anshul96go commented 5 years ago

Thanks @jaymon0703 for review Please go ahead with the required structuring. I will perform the remaining edits by this weekend

jaymon0703 commented 5 years ago

Very well, i will only make these adjustments once the function is merged into 54_algotca.

Please submit the PR for the trading price performance commit to the 54_algotca branch as opposed to master? This will be a requirement if we are to ultimately merge that function into the feature branch 54_algotca.

image

The typo fix PR can go straight to master.

Thanks!