✨ Add Metrics TimeSeries #90
Conversation
| @@ -20,7 +20,7 @@ | |||
| @dataclass | |||
| class PortfolioAsset: | |||
| allocation: float | |||
| volatility: float | |||
| metrics: dict[str, float] | |||
There was a problem hiding this comment.
It's a good thing that this PR suggests a change on the backend only, so let's try to make it mergeable by itself, without needing to wait for the frontend changes.
To keep this new backend version compatible with the frontend, we need to keep the volatility key here. Let's just add metrics next to it.
| @@ -24,4 +24,4 @@ def make_hist_price_series( | |||
| (price.value for price in prices), | |||
| index=pd.Index((price.timestamp for price in prices), name="timestamp"), | |||
| name=f"{token_symbol} historical price", | |||
| ) | |||
| ).sort_index() | |||
There was a problem hiding this comment.
Just for my knowledge, did you notice that the indices were not sorted before introducing this .sort_index()?
There was a problem hiding this comment.
Yes, they were sorted in descending order for Prices.
The biggest consequence being that the std_dev calculation was incorrect.
There was a problem hiding this comment.
Here is the summary representation of a given price dataframe before and after the change:
Before:
After:
Indeed the std_dev calcuation, albeit being very similar, is not the same. Nevertheless, I guess the volatility calculation stays the same.
This index-sorting fix has fairly wide-ranging consequences. We might need to look into git history to look for when a bug was introduced (think git bisect). A good rule of thumb is thus to always move such a fix to a separate independent commit.
acemasterjb
force-pushed
the
feature/metrics_ts
branch
from
dfa494e to
bbb002c
Compare
acemasterjb
force-pushed
the
feature/metrics_ts
branch
from
bbb002c to
3c386ad
Compare
| @@ -24,4 +24,4 @@ def make_hist_price_series( | |||
| (price.value for price in prices), | |||
| index=pd.Index((price.timestamp for price in prices), name="timestamp"), | |||
| name=f"{token_symbol} historical price", | |||
| ) | |||
| ).sort_index() | |||
There was a problem hiding this comment.
Here is the summary representation of a given price dataframe before and after the change:
Before:
After:
Indeed the std_dev calcuation, albeit being very similar, is not the same. Nevertheless, I guess the volatility calculation stays the same.
This index-sorting fix has fairly wide-ranging consequences. We might need to look into git history to look for when a bug was introduced (think git bisect). A good rule of thumb is thus to always move such a fix to a separate independent commit.
This timeseries will have both `std_dev` and `returns` observations :bug: Validate `returns` and `std_dev` timeseries :bug: Sort the index of price series This fixes an issue where the std_dev ts would be incorrect, since the rolling window calcs are applied on a decending ordered ts. This means that the last seven days would not have std_dev observations, but the first seven would. :rewind: Reintroduce asset property for frontend
acemasterjb
force-pushed
the
feature/metrics_ts
branch
from
3c386ad to
2f4f7a9
Compare
Summary
Fixes #80
Details
This PR exposes the historical 7-day avg standard deviation and historical returns to the
portfolioendpoint.The
volatilityproperty of each asset in this endpoint response will be replaced withmetricscontaining the historical std_dev asvolatilityand returns asreturnsas timeseries.Further Improvements
The graph on the frontend will need to also plot these timeseries on the three timeframes (1-yr, 3-months, 1-month).