Skip to content
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

Fix portfolio and benchmark length matching #4218

Merged
merged 22 commits into from
Mar 9, 2023

Conversation

northern-64bit
Copy link
Contributor

Description

Closes #4201 by fixing the length matching performed in the "bench" command.

How has this been tested?

Running the command and making sure the results stay the same

  • Make sure affected commands still run in terminal
  • Ensure the SDK still works
  • Check any related reports

Checklist:

Others

  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.

@northern-64bit northern-64bit added bug Fix bug portfolio Portfolio menu labels Feb 11, 2023
@reviewpad reviewpad bot added the feat XS Extra small feature label Feb 11, 2023
Copy link
Contributor

@montezdesousa montezdesousa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC we are assuming that if DataFrames share the same length the dates match, this might not be always the case, right?

Or even if they have different length is it guaranteed that the smallest is a subset of the largest?

I tried this example and the resulting DataFrames don't share the same length

from openbb_terminal.portfolio.portfolio_helper import make_equal_length
import pandas as pd
df0 = pd.DataFrame({'returns': [0.4, 0.6, 0.8]}, index=[1, 3, 4])
df1 = pd.DataFrame({'returns': [0.3, 0.5]}, index=[2, 3])
df0_new, df1_new = make_equal_length(df0, df1)
df0_new, df1_new

If the case above is an issue we might use pandas inner merge and then provide each DataFrame individually.

@jmaslek
Copy link
Collaborator

jmaslek commented Feb 14, 2023

IIUC we are assuming that if DataFrames share the same length the dates match, this might not be always the case, right?

Or even if they have different length is it guaranteed that the smallest is a subset of the largest?

I tried this example and the resulting DataFrames don't share the same length

from openbb_terminal.portfolio.portfolio_helper import make_equal_length
import pandas as pd
df0 = pd.DataFrame({'returns': [0.4, 0.6, 0.8]}, index=[1, 3, 4])
df1 = pd.DataFrame({'returns': [0.3, 0.5]}, index=[2, 3])
df0_new, df1_new = make_equal_length(df0, df1)
df0_new, df1_new

If the case above is an issue we might use pandas inner merge and then provide each DataFrame individually.

So I think the tricky part is holidays on one exchange. I am not sure what the correct way to report that would be. My gut says to do an outer join and then just backfill (and just assume that a couple days of 0 returns doesnt mess with statistics or correlations)

@montezdesousa
Copy link
Contributor

IIUC we are assuming that if DataFrames share the same length the dates match, this might not be always the case, right?
Or even if they have different length is it guaranteed that the smallest is a subset of the largest?
I tried this example and the resulting DataFrames don't share the same length

from openbb_terminal.portfolio.portfolio_helper import make_equal_length
import pandas as pd
df0 = pd.DataFrame({'returns': [0.4, 0.6, 0.8]}, index=[1, 3, 4])
df1 = pd.DataFrame({'returns': [0.3, 0.5]}, index=[2, 3])
df0_new, df1_new = make_equal_length(df0, df1)
df0_new, df1_new

If the case above is an issue we might use pandas inner merge and then provide each DataFrame individually.

So I think the tricky part is holidays on one exchange. I am not sure what the correct way to report that would be. My gut says to do an outer join and then just backfill (and just assume that a couple days of 0 returns doesnt mess with statistics or correlations)

sounds good

@northern-64bit
Copy link
Contributor Author

IIUC we are assuming that if DataFrames share the same length the dates match, this might not be always the case, right?
Or even if they have different length is it guaranteed that the smallest is a subset of the largest?
I tried this example and the resulting DataFrames don't share the same length

from openbb_terminal.portfolio.portfolio_helper import make_equal_length
import pandas as pd
df0 = pd.DataFrame({'returns': [0.4, 0.6, 0.8]}, index=[1, 3, 4])
df1 = pd.DataFrame({'returns': [0.3, 0.5]}, index=[2, 3])
df0_new, df1_new = make_equal_length(df0, df1)
df0_new, df1_new

If the case above is an issue we might use pandas inner merge and then provide each DataFrame individually.

So I think the tricky part is holidays on one exchange. I am not sure what the correct way to report that would be. My gut says to do an outer join and then just backfill (and just assume that a couple days of 0 returns doesnt mess with statistics or correlations)

sounds good

It has now been fixed by applying outer join and just replacing all NaNs with zeros.

@jmaslek
Copy link
Collaborator

jmaslek commented Feb 20, 2023

Loading the example portfolio is now invalid:

2023 Feb 19, 20:09 (🦋) /portfolio/ $ load --example

Loading an example, please type `about` to learn how to create your own Portfolio Excel sheet.

         Loading benchmark:  50%|████████████████████████████████████████████████████████████████████▌                                                                    | 2/4 [00:00<00:00, 10.45it/s]Error: 'Series' object has no attribute 'columns'

                                                                                                                                                                                                        
Portfolio: OpenBB Example Portfolio
Risk Free Rate: 0.00%
Benchmark: SPDR S&P 500 ETF Trust (SPY)

@northern-64bit
Copy link
Contributor Author

Loading the example portfolio is now invalid:

2023 Feb 19, 20:09 (🦋) /portfolio/ $ load --example

Loading an example, please type `about` to learn how to create your own Portfolio Excel sheet.

         Loading benchmark:  50%|████████████████████████████████████████████████████████████████████▌                                                                    | 2/4 [00:00<00:00, 10.45it/s]Error: 'Series' object has no attribute 'columns'

                                                                                                                                                                                                        
Portfolio: OpenBB Example Portfolio
Risk Free Rate: 0.00%
Benchmark: SPDR S&P 500 ETF Trust (SPY)

Fixed :)

Copy link
Contributor

@montezdesousa montezdesousa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are commands breaking for some reason, like summary, some on metrics, yret

Run python terminal.py -t 33 -v to check. alloc is broken, but apparently it's on develop branch already

@northern-64bit
Copy link
Contributor Author

Hello @montezdesousa! Can you please review the PR so that we can merge it. Thanks!

Copy link
Contributor

@montezdesousa montezdesousa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry must have missed this one, tks for reminding

@montezdesousa montezdesousa enabled auto-merge March 9, 2023 11:41
@montezdesousa montezdesousa added this pull request to the merge queue Mar 9, 2023
Merged via the queue into OpenBB-finance:develop with commit 03c18f5 Mar 9, 2023
@northern-64bit northern-64bit deleted the hotfix/bug-#4201 branch March 9, 2023 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fix bug feat XS Extra small feature portfolio Portfolio menu
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Portfolio/Bench - Error "[Timestamp('2023-01-16 00:00:00')] not in index"
3 participants