-
Notifications
You must be signed in to change notification settings - Fork 133
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Finance metrics and median calculation #1289
Conversation
Job Test CentOS 7 on b65d0c4 : invalidated by @joshua-cogliati-inl failed in set python environment. |
@JiaZhou-PU ready to be reviewed? |
Yep |
...ork/PostProcessors/BasicStatistics/gold/basicStatisticsGeneral/PointSetPostProcTest_dump.xml
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did my first review. In general it looks reasonable, but I did have some comments and questions.
\item \textbf{sharpeRatio}: the Sharpe Ratio, measures the performance of an investment. It is defined as the historical returns of the investment, divided by the standard deviation of the investment(Volatility). | ||
\item \textbf{sortinoRatio}: the Sortino ratio, measures the risk-adjusted return of an investment asset. Discounts the excess return of a portfolio above a target threshold by the volatility of downside returns. If this quantity is inputted as \textit{sortinoRatio} the threshold for separate upside and downside value will assign as $0$. Otherwise the user can specify this quantity with a parameter \xmlAttr{threshold='X'}, where the \xmlAttr{X} represents the requested threshold \xmlAttr{median} or \xmlAttr{zero}. | ||
|
||
\item \textbf{gainLossRatio}: the gain-loss ratio, discounts the first-order higher partial moment of a portfolio's returns, by the first-order lower partial moment of a portfolio's returns. If this quantity is inputted as \textit{gainLossRatio} the threshold for separate upside and downside value will assign as $0$. Otherwise the user can specify this quantity with a parameter \xmlAttr{threshold='X'}, where the \xmlAttr{X} represents the requested threshold \xmlAttr{median} or \xmlAttr{zero}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"threshold for separate upside and downside value will assign as
Does this mean "the default threshold for the separate upside and downside value is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the financial data put in to calculation is usually the percentage change based on the original price. So 0 would be an ideal threshold, somehow in other calculation you could assign median as the gain or lose threshold.
\begin{equation} | ||
ES_\alpha = -\frac{1}{\alpha} \int_0^\alpha \operatorname{VaR}_\gamma(X) \, d\gamma | ||
\end{equation} | ||
\item \textbf{valueAtRisk}: the value at risk for investments. Estimates the maximum possible loss after exclude worse outcomes whose combined probability is at most $\alpha$. If this quantity is inputted as \textit{ValueAtRisk} the $\alpha$ value will assign as $5\%$. Otherwise the user can specify this quantity with a parameter \xmlAttr{threshold='X'}, where the \xmlAttr{X} represents the requested $\alpha$ value (a floating point value between 0.0 and 1.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do 0.0 to 1.0 represent 0% to 100%? (Also for expectedShortfall)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's basically like a quantile.
@@ -1,2 +1,2 @@ | |||
skew_x,vc_x,percentile_5_x,percentile_95_x,mean_x,kurt_x,median_x,max_x,min_x,samp_x,var_x,sigma_x | |||
0.11423075075,0.188785800293,5.91996682062,8.1183566722,7.28508256545,-3.67853432603,6.33133089078,8.77067587821,5.91996682062,4.0,1.89150549387,1.37532014232 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are there changes in basic statistics results?
Note that this is REQUIREMENT test, so this needs to be checked by Andrea as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as COB Chair, I approve the modification/regolding of this test.
Thank you Josh for pointing this out.
An analytical test should be added
This changes results (since this changes the median and percentile calculation). Can you write a proposed email explaining this? |
I am sorry about asking for more work, but if we are changing the percentages and the median value, we really need to add a test where we pass in a known vector and check that we get out the exact calculated values, so we can check that the values are really correct this time. |
@joshua-cogliati-inl No problem Josh, I will send a test to you and explain. |
Job Mingw Test on c861477 : invalidated by @alfoa |
tests/framework/PostProcessors/BasicStatistics/test_median_and_percentile.xml
Outdated
Show resolved
Hide resolved
@PaulTalbot-INL @dylanjm this PR causes Diff Failures on HERON plugin tests. |
@JiaZhou-PU can you update the issue #1225 (#1225) regarding the new features. I will update the Wiki page (Changelog) at https://github.com/idaholab/raven/wiki/RAVEN-Time-Line |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved
tests/framework/PostProcessors/BasicStatistics/test_median_and_percentile.xml
Show resolved
Hide resolved
@joshua-cogliati-inl I approved Jia PR. I think we are good to go. |
Checklist passed... PR can be approved... Comments have been addressed... Merging... |
Plugin tests (HERON Ref Issue idaholab/HERON#42 ) are failing. The HERON team has been notified via email |
I think we still need to send out an email to the users' list since this changes calculation results. |
Yes agree! @JiaZhou-PU can you draft an email and send it? |
Wait, so did we completely remove the option to take the median from the population for odd-numbered sets? I must have missed some of the conversations.
EDIT: I checked 1339, apparently the answer is yes, we've removed the existing option until someone adds it back in. Just in case someone else goes looking for the same question.
|
see #1339 1339 |
Pull Request Description
What issue does this change request address? (Use "#" before the issue to link it, i.e., #42.)
closes #1259
What are the significant changes in functionality due to this change request?
Add financial metric:
Sharpe ratio risk-free rate as 0
Sortino ratio
gain loss ratio:
the first-order higher partial moment of a portfolio's returns, divided by the first-order lower partial moment of a portfolio's returns.
value at risk
conditional value at risk
Fixed the percentile calculation with shift one index upper.
For Change Control Board: Change Request Review
The following review must be completed by an authorized member of the Change Control Board.
<internalParallel>
to True.raven/tests/framework/user_guide
andraven/docs/workshop
) have been changed, the associated documentation must be reviewed and assured the text matches the example.