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

Refactoring econometrics for SDK usability #3379

Merged
merged 27 commits into from
Nov 15, 2022

Conversation

jmaslek
Copy link
Collaborator

@jmaslek jmaslek commented Nov 10, 2022

The econometrics menu is nice, but the SDK usage is not friendly because how we do it in the terminal. This menu needs some refactoring so that the SDK is usable. Still love you @JerBouma.

Things done:

Screen Shot 2022-11-10 at 5 19 52 PM

    Returns
    -------
    float
        Test statistic of the Durbin Watson test.

    Notes
    ------
    When using chart = True, the dependent variable in the regression must be passed to view the residuals

Screen Shot 2022-11-11 at 11 11 45 AM

Screen Shot 2022-11-13 at 1 36 03 PM

@jmaslek jmaslek added refactor Refactor code platform OpenBB Platform labels Nov 10, 2022
@jmaslek jmaslek marked this pull request as ready for review November 10, 2022 22:20
@jmaslek
Copy link
Collaborator Author

jmaslek commented Nov 10, 2022

There are a couple functions that need some cleaning up for the sdk, but if anyone wants to review this overnight feel free :)

@JerBouma
Copy link
Contributor

The econometrics menu is nice, bla bla bla love you @JerBouma.

Thank you <3

@jmaslek
Copy link
Collaborator Author

jmaslek commented Nov 11, 2022

I just have panel left

@JerBouma
Copy link
Contributor

Make sure by the way that if you do chart=True (or anything else) that it doesn't print the richt table. Those should always be DataFrames.

@jmaslek
Copy link
Collaborator Author

jmaslek commented Nov 14, 2022

Make sure by the way that if you do chart=True (or anything else) that it doesn't print the richt table. Those should always be DataFrames.

Ummm that's the desired behavior to print the nice charts when chart = True. Doing otherwise would be a whole refactor.

@JerBouma
Copy link
Contributor

Make sure by the way that if you do chart=True (or anything else) that it doesn't print the richt table. Those should always be DataFrames.

Ummm that's the desired behavior to print the nice charts when chart = True. Doing otherwise would be a whole refactor.

Just making sure we don't mean different things, I mean that if you use chart=True I don't want to see the table too, see:

image

Whereas this screenshot also shows the Rich table. That looks wrong.

image

@jmaslek
Copy link
Collaborator Author

jmaslek commented Nov 14, 2022

Just making sure we don't mean different things, I mean that if you use chart=True I don't want to see the table too, see:

Not possible with the current architecture.

@JerBouma
Copy link
Contributor

Ok ok

@DidierRLopes
Copy link
Collaborator

Ideally we would have the raw=True when we want to output the table. Is this not possible? @jmaslek @JerBouma

Spitting both is very suboptimal, imo. Would be better to just spit the chart than both

@JerBouma
Copy link
Contributor

Ideally we would have the raw=True when we want to output the table. Is this not possible? @jmaslek @JerBouma

Spitting both is very suboptimal, imo. Would be better to just spit the chart than both

The rich table shouldn't be an option in the first place, you can rely on the default output (without chart=True) which is a DataFrame object. But if this requires too big of an architectural change it isn't too big of a deal for now.

@jmaslek
Copy link
Collaborator Author

jmaslek commented Nov 14, 2022

Ideally we would have the raw=True when we want to output the table. Is this not possible? @jmaslek @JerBouma

Spitting both is very suboptimal, imo. Would be better to just spit the chart than both

I think this gets into the question of the default output. In this menu its primarily tables so we want raw=True by default. You can actually pass plot=False in these cases. So I think its a matter of what the default output is

@jmaslek
Copy link
Collaborator Author

jmaslek commented Nov 14, 2022

The rich table shouldn't be an option in the first place, you can rely on the default output (without chart=True) which is a DataFrame object. But if this requires too big of an architectural change it isn't too big of a deal for now.

If you are in a script, you wouldnt get the default output and could instead see a table with view. So if you want the default output, dont use the _view.

@reviewpad reviewpad bot added the feat XL Extra Large feature label Nov 14, 2022
@jmaslek jmaslek merged commit 4495d4d into OpenBB-finance:main Nov 15, 2022
@jmaslek jmaslek deleted the refactor_econometrics branch November 15, 2022 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment