Add bar support to compute Portfolio pnls #2239
Conversation
faysou
force-pushed
the
barpnl
branch
2 times, most recently
from
c383c01 to
1d70ff4
Compare
| bar.ts_init, | ||
| ) | ||
|
|
||
| self._cache.add_quote_tick(tick) |
There was a problem hiding this comment.
Wondering whether adding the quote to the cache can have any side effects when strategies load quotes from the cache.
There was a problem hiding this comment.
it could be the case if quotes and external bars are used at the same time, but portfolio currently uses the cache to read quotes so there wasn't another easy way.
my assumption is that if somone has access to quote, he doesn't need external bars and can aggregate quotes instead. another possibility would be to add an option to consider bars or not in portfolio.
the created quotetick isn't sent on the message bus though, it's just stored in the cache, so I doubt it can have an impact except if someone uses quotes and external bars at the same time, in this case a quotetick could be overriden by this dummy quotetick in the cache. that's an edge case I suppose, but yes, could be improved.
I'll add an option to enable or disable portfolio updates with bars, this way we can avoid this edge case.
There was a problem hiding this comment.
Please think this through carefully, guys 🙏
I'd really like to help you think through the implications, but I don't have deep enough knowledge at this moment.
Generally speaking, it's very important that we maintain the generic functionality of the entire framework and don't introduce any assumptions that would be difficult to explain or understand later (for example, assuming that when someone uses certain data, they "shouldn't" need other data). People can configure all sorts of data and combinations, and generally, it should always work.
Please just emphasize making sure we don't create any limitations (where certain types of data conflict with each other in the Cache).
The goal is for Portfolio to be able to work 100% with bar-data while simultaneously maintaining all original flexibility without additional limitations or assumptions.
I completely understand this isn't a trivial matter, which is why I'm really relying on you more experienced folks.
I'll help and support you as much as I can with thorough testing and documenting this new functionality ❤️ 👍
There was a problem hiding this comment.
you're right, I'll do something about it, I wouldn't like to write something with a logical flaw
There was a problem hiding this comment.
Please think this through carefully, guys 🙏 I'd really like to help you think through the implications, but I don't have deep enough knowledge at this moment.
Generally speaking, it's very important that we maintain the generic functionality of the entire framework and don't introduce any assumptions that would be difficult to explain or understand later (for example, assuming that when someone uses certain data, they "shouldn't" need other data). People can configure all sorts of data and combinations, and generally, it should always work.
Please just emphasize making sure we don't create any limitations (where certain types of data conflict with each other in the Cache).
The goal is for Portfolio to be able to work 100% with bar-data while simultaneously maintaining all original flexibility without additional limitations or assumptions.
I completely understand this isn't a trivial matter, which is why I'm really relying on you more experienced folks. I'll help and support you as much as I can with thorough testing and documenting this new functionality ❤️ 👍
It is great to keep all components 100% available
faysou
force-pushed
the
barpnl
branch
2 times, most recently
from
11cebd4 to
7cc95b2
Compare
faysou
changed the title
faysou
force-pushed
the
barpnl
branch
3 times, most recently
from
3ea513c to
904f284
Compare
faysou
changed the title
|
I've taken into account all comments above and updated the code just now |
Pull Request
Add support for bars in Portfolio.
Subscribing to External bars only which should be the most granular as less granular bars aggregated from bars or quote ticks would probably be internal.
Fixing: #2236
Type of change
How has this change been tested?
Added one test for bars in Portfolio