Ppu history #365
Merged
Ppu history #365
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Use the new `.calc_[be_price/size]()` methods when serializing to and from the `pps.toml` format and add an audit method which will warn about mismatched values and assign the clears table calculated values pre-write. Drop the `.lifo_update()` method and instead allow both `.size`/`.be_price` properties to exist (for non-ledger related uses of `Position`) alongside the new calc methods and only get fussy about *what* the properties are set to in the case of ledger audits. Also changes `Position.update()` -> `.add_clear()`.
The original implementation of `.calc_be_price()` wasn't correct since the real so called "price per unit" (ppu), is actually defined by a recurrence relation (which is why the original state-updated `.lifo_update()` approach worked well) and requires the previous ppu to be weighted by the new accumulated position size when considering a new clear event. The ppu is the price that above or below which the trader takes a win or loss on transacting one unit of the trading asset and thus it is the true "break even price" that determines making or losing money per fill. This patches fixes the implementation to use trailing windows of the accumulated size and ppu to compute the next ppu value for any new clear event as well as handle rare cases where the "direction" changes polarity (eg. long to short in a single order). The new method is `Position.calc_ppu()` and further details of the relation can be seen in the doc strings. This patch also includes a wack-ton of clean ups and removals in an effort to refine position management api for easier use in new backends: - drop `updaate_pps_conf()`, `load_pps_from_toml()` and rename `load_trands_from_ledger()` -> `load_pps_from_ledger()`. - extend `PpTable` to have a `.to_toml()` method which returns the active set of positions ready to be serialized to the `pps.toml` file which is collects from calling, - `PpTable.dump_active()` which now returns double dicts of the open/closed pp object maps. - make `Position.minimize_clears()` now iterate the clears table in chronological order (instead of reverse) and only drop fills prior to any zero-size state (the old reversed way can result incorrect history-size-retracement in cases where a position is lessened but not completely exited). - drop `Position.add_clear()` and instead just manually add entries inside `.update_from_trans()` and also add a `accum_size` and `ppu` field to ever entry thus creating a position "history" sequence of the ppu and accum size for every position and prepares for being and to show "position lifetimes" in the UI. - move fqsn getting into `Position.to_pretoml()`.
goodboy
added a commit
that referenced
this pull request
Jul 26, 2022
If we don't have a pos table built out already (in mem) we can't figure out the likely dst asset (since there's no pair entry to guide us) that we should use to search for withdrawal transactions; so move it later. Further this ports to the new api changes in `piker.pp`` that will land with #365.
goodboy
added a commit
that referenced
this pull request
Jul 27, 2022
If we don't have a pos table built out already (in mem) we can't figure out the likely dst asset (since there's no pair entry to guide us) that we should use to search for withdrawal transactions; so move it later. Further this ports to the new api changes in `piker.pp`` that will land with #365.
guilledk
previously approved these changes
Jul 27, 2022
There was a problem hiding this comment.
"could we maybe just drop the top level .size and be_price values and instead just arrange the clears table to make these rolling values more obvious?"
Sure, sounds good.
"it follows that maybe we should adjust the column order for each clears entry maybe to: dt, ppu, accum_size, price, size, cost, tid?"
Agreed.
... "my only concern is then the clears table entries will be a bit cramped horizontally?"
Mmmm not sure, id have to see an example.
Quick review of the code looks great.
goodboy
added
accounting
goodboy
added a commit
that referenced
this pull request
Jul 27, 2022
If we don't have a pos table built out already (in mem) we can't figure out the likely dst asset (since there's no pair entry to guide us) that we should use to search for withdrawal transactions; so move it later. Further this ports to the new api changes in `piker.pp`` that will land with #365.
goodboy
added a commit
that referenced
this pull request
Jul 27, 2022
If we don't have a pos table built out already (in mem) we can't figure out the likely dst asset (since there's no pair entry to guide us) that we should use to search for withdrawal transactions; so move it later. Further this ports to the new api changes in `piker.pp`` that will land with #365.
goodboy
added a commit
that referenced
this pull request
Jul 27, 2022
If we don't have a pos table built out already (in mem) we can't figure out the likely dst asset (since there's no pair entry to guide us) that we should use to search for withdrawal transactions; so move it later. Further this ports to the new api changes in `piker.pp`` that will land with #365.
goodboy
added a commit
that referenced
this pull request
Jul 30, 2022
If we don't have a pos table built out already (in mem) we can't figure out the likely dst asset (since there's no pair entry to guide us) that we should use to search for withdrawal transactions; so move it later. Further this ports to the new api changes in `piker.pp`` that will land with #365.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proper fixes and terminology to get us ready for historical position lifetimes as described in #345.
More to come in terms of explanation... or not 😂
pps.tomlformat changes:.sizeandbe_pricevalues ?Positiontype internally as well and doesn't track all theclears (since that would be more msging overhead) so I think maybe having these is fine but we should probably
change
.be_price->.ppu?which would be easiest to read if we sort clears reverse chronological so that the most recent trades (and thus their
ppuandaccum_sizeentries would be closest to the top?dt, ppu, accum_size, price, size, cost, tid?[ ] i was hoping to have an indent per account (mentioned it in Positioning system refinements: paper engine, file writing, toml style and perf #345 as part of a custom encoder) such that it's distinguishable which pps are grouped together by backend, my only concern is then the clears table entries will be a bit cramped horizontally?delaying this one again