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

Adding stock price comparison via -c flag #1745

Merged

Conversation

Avani1994
Copy link
Contributor

@Avani1994 Avani1994 commented Apr 27, 2022

Adding stock price comparison via -c flag

Added a compare flag in twitter sentiment analysis to allow see the movements in stock price alongside the sentiment. It's just a small enhancement to the sentiment command under /stocks/ba/.

I just felt stock price movements along with sentiment will be really helpful from customer point of view.

None of the dependencies are required for this change.

How has this been tested?

Couldn't run tests, as it was asking to uninstall brotlipy and conda uninstall didn't do the job, hopefully CI should catch failing tests if any

  • Provide instructions so we can reproduce.
    $/stocks/ba sentiment -c True

Output:
Screen Shot 2022-04-26 at 11 30 44 PM

  • Please also list any relevant details for your test configuration. - None

Checklist:

Copy link
Collaborator

@DidierRLopes DidierRLopes left a comment

Choose a reason for hiding this comment

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

Nice one @Avani1994 !!

I put my comments below 😄

# This plot has 3 axis
if external_axes is None:
_, axes = plt.subplots(
3, 1, sharex=False, figsize=(12, 7), dpi=cfg_plot.PLOT_DPI
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here the fig size should be plot_autoscale()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, I tried this, but then with three figs, the individual plots are quiet small. Wouldn't that be a problem?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That may be because our default is not ideal, but this should be able to be configurable by the user

_, axes = plt.subplots(
2, 1, sharex=True, figsize=plot_autoscale(), dpi=cfg_plot.PLOT_DPI
# get stock end price for each corresponding day if compare == True
if not compare:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Because the ax1 and ax2 do not change regardless of whether we display stock price or not. We should do something like the following pseudo-code:

if compare:
    ax1,ax2,ax3 are created
else:
   ax1,ax2 are created

populate ax1 and ax2 (this avoids repeated code)

if compare:
   populate ax3 with stock price

set theme

This allows to minimize duplicated code and makes it easier for debug

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see yeah this makes sense to me, we have to just write if compare twice, i thought that wouldn't be good idea, but yeah we can do this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes it easier from the readability perspective and maintenance as well since there isn't repeated code

parser.add_argument(
"-c",
"--compare",
action="store",
Copy link
Collaborator

Choose a reason for hiding this comment

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

For true/false flags you can use here "store_true" as action and remove both the type and the default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's good advice :)

@DidierRLopes DidierRLopes added the enhancement Enhancement label Apr 27, 2022
@DidierRLopes
Copy link
Collaborator

@Avani1994 for the tests to pass you need to run black linting to the helper_funcs

@piiq this is also failing in the compose_export_path do you know why?

@piiq
Copy link
Contributor

piiq commented Apr 27, 2022

thank you @Avani1994

re: compose_export_path
The error says NameError: name 'Tuple' is not defined.
Add Tuple on line 7 in openbb_terminal/helper_funcs.py to resolve, although the import is available in main so it that line wasn't edited it should work fine once the PR branch is synced with main.

@@ -4,8 +4,8 @@
import argparse
import logging
from pathlib import Path
from typing import List, Tuple, Union
Copy link
Contributor

Choose a reason for hiding this comment

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

this is why tests are failing

@DidierRLopes
Copy link
Collaborator

@Avani1994 you just need to fix the test for the -c flag 😄

Copy link
Collaborator

@DidierRLopes DidierRLopes left a comment

Choose a reason for hiding this comment

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

Nice work @Avani1994 !

@DidierRLopes DidierRLopes merged commit b73e091 into OpenBB-finance:main May 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants