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

Adds a matlab like hold on to plot on same axis #5173

Merged
merged 35 commits into from
Jul 19, 2023
Merged

Conversation

jmaslek
Copy link
Collaborator

@jmaslek jmaslek commented Jun 22, 2023

Definitely was not juan telling me to do.

But this should be super helpful for scripts to avoid having to manually overlay.

There will be some growing pains with different types of charts (ta charts vs candles vs lines and pies etc)

@reviewpad reviewpad bot added the feat S Small T-Shirt size Feature label Jun 22, 2023
@jmaslek jmaslek added the do not merge Label to prevent pull request merge label Jun 22, 2023
@jmaslek
Copy link
Collaborator Author

jmaslek commented Jul 7, 2023

@tehcoderer why you break mypy on me

@jmaslek
Copy link
Collaborator Author

jmaslek commented Jul 17, 2023

@DidierRLopes here is a fun script :)

$STOCKS=AAPL,AMZN,MSFT,TSLA

stocks/load $STOCKS[0]/candle
fa
hold on -s
foreach $$tick in $STOCKS:
income -t $$tick -l 60 -q --plot revenue
end
hold off
hold on 
foreach $$tick in $STOCKS:
cash -t $$tick -l 60 -q --plot net_income
end
hold off

@jmaslek jmaslek removed the do not merge Label to prevent pull request merge label Jul 17, 2023
@DidierRLopes
Copy link
Collaborator

This is going to be huge. A few comments below,

If I do:

2023 Jul 18, 12:03 [didier] :butterfly: /stocks/fa/ $ hold on
2023 Jul 18, 12:03 [didier] :butterfly: /stocks/fa/ $ income -q -l 30 --source Polygon --plot revenue
2023 Jul 18, 12:03 [didier] :butterfly: /stocks/fa/ $ income -q -l 30 --source Polygon --plot revenue -t MSFT
2023 Jul 18, 12:03 [didier] :butterfly: /stocks/fa/ $ hold off

I get:

image

1/ I should be able to select my own legend otherwise they don't make much sense. This also solves issues when charts don't have a legend.

2/ I don't know which axis corresponds to each plot. The axes should be of the color of the plot it corresponds to

3/ The font on the left axis is smaller for some reason

4/ The title should be able to be customizable, so I can do things like "comparing revenues between tech companies" and similar

5/ The notion of command on the top right should be hidden since it doesn't make sense anymore with multiple ones being used.

We need to have a more robust solution to handle edge cases, which will happen.

If I run hold on/ycrv/cp/hold off, I only get ycrv with maturity in x. But if I do hold on/cp/ycrv/hold off then I get the commercial paper and a weird yield curve graph. This is because ycrv index doesn't match cp type.

I suggest that we check whether an output has a timeseries as an index type. If not and cannot be converted into one, I suggest we raise a warning saying that the output is not a timeseries and therefore will not be added to the chart.

@deeleeramone
Copy link
Contributor

bet'cha couldn't make it work like the --ma flag. :P

(🦋) /stocks/ $ hold on


(🦋) /stocks/ $ candle

Hold functionality not supported with candle.

If it doesn't work with candle, can we convert candle to a line, somehow, so that it does work?

Something like,

hold on
plot --line high
plot --line close --sameaxis

I would vote for the argument name,sameaxis, to be either lower camelCase or snake_case.

@jmaslek
Copy link
Collaborator Author

jmaslek commented Jul 19, 2023

1/ I should be able to select my own legend otherwise they don't make much sense. This also solves issues when charts don't have a legend.

Donezo. If a legend is not provided, then it will default to either the command location or wont show (in the case of the function not having a legend).

2/ I don't know which axis corresponds to each plot. The axes should be of the color of the plot it corresponds to

I couldnt figure out the spines, but I made the tick colors the same as the line color (except for the primary one, which is always white)

3/ The font on the left axis is smaller for some reason

I dont see where we actually set the font size in our _views, so I adjusted it by hand to try to match a few functions. Will likely find some cases i appears different sizes

4/ The title should be able to be customizable, so I can do things like "comparing revenues between tech companies" and similar

Donezo. hold off --title comparing revenues between tech companies

5/ The notion of command on the top right should be hidden since it doesn't make sense anymore with multiple ones being used.

Donezo

We need to have a more robust solution to handle edge cases, which will happen.

If I run hold on/ycrv/cp/hold off, I only get ycrv with maturity in x. But if I do hold on/cp/ycrv/hold off then I get the commercial paper and a weird yield curve graph. This is because ycrv index doesn't match cp type.

I suggest that we check whether an output has a timeseries as an index type. If not and cannot be converted into one, I suggest we raise a warning saying that the output is not a timeseries and therefore will not be added to the chart.

As we said, there should be some level of user error. We are likely to see some weird things when we are dealing with date times that are either - wrong types (datetime vs string) or different frequencies etc.

@jmaslek
Copy link
Collaborator Author

jmaslek commented Jul 19, 2023

bet'cha couldn't make it work like the --ma flag. :P

(🦋) /stocks/ $ hold on


(🦋) /stocks/ $ candle

Hold functionality not supported with candle.

The actual underlying issue is that the candle is generated from Juans PlotlyTA class, which has no inheritance from the OpenBBFigure. The hold logic is in the OpenBBFigure. Since this relies on plotting on the same axes, figures that have subplots will fail (like ta or qa functions).

If it doesn't work with candle, can we convert candle to a line, somehow, so that it does work?

Something like,

hold on
plot --line high
plot --line close --sameaxis

I could edit the warning to say use qa/pick close/plot instead

I would vote for the argument name,sameaxis, to be either lower camelCase or snake_case.

Breaks convention. If anything --same-axis but thats one more character.

@reviewpad reviewpad bot added feat M Medium T-Shirt size feature and removed feat S Small T-Shirt size Feature labels Jul 19, 2023
@jmaslek jmaslek enabled auto-merge July 19, 2023 23:11
@jmaslek jmaslek force-pushed the feature/plot-hold branch from 92d1942 to c4f4706 Compare July 19, 2023 23:18
@jmaslek jmaslek added this pull request to the merge queue Jul 19, 2023
Merged via the queue into develop with commit 8b00778 Jul 19, 2023
@jmaslek jmaslek deleted the feature/plot-hold branch July 25, 2023 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat M Medium T-Shirt size feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants