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

Feature/yieldcurve #1734

Merged
merged 21 commits into from
May 17, 2022
Merged

Feature/yieldcurve #1734

merged 21 commits into from
May 17, 2022

Conversation

montezdesousa
Copy link
Contributor

Description

This PR adds a new feature to the 'economy' menu. The 'yieldcurve' command will allow users to display and export the yield curve from 3M to 30Y, if exists, for several countries. From my understanding the 'yield' command only works for US. This API officially supports 74 countries. (https://www.investing.com/rates-bonds/world-government-bonds)

Data comes from 'Investing.com' which has nice module 'investpy' that lets you pull real-time data for several instruments. It also provides other interesting information that could be easily converted into features (e.g. economic calendar). Please check: https://investpy.readthedocs.io/api.html

Example as implemented:
image

I think it would be nice to add plot option to these, to graph yield values by tenor - typical yield curve plot.
image

Another cool option would be to create a matrix command where user can see curve spreads for specific tenor. Later on this could be spun off into a new 'bonds' menu.

With a structure like this mock example for 10y rates where we can see that AT (Austria) trades -5.2 basis points below BE (Belgium). Or that PT (Portugal) trades 4.4bps above ES (Spain). This is commonly used by govt bond market participants.
image

Happy to get your feedback

How has this been tested?

Added simple choice tests to the investingcom_model.py, investingcom_view_py and economy_controller.py
Still working on tests with vcr. It's failing to record the HTTP request for some reason.

Checklist:

Others

  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • My code passes all the checks pylint, flake8, black, ... To speed up development you should run pre-commit install.
  • New and existing unit tests pass locally with my changes. You can test this locally using pytest tests/....

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!! 🚀

Here is my review:

  • Instead of having a new command let's put this under yield and use this source by default as the one that is currently available relies on FRED.

  • Let's put then Source: Investpy/FRED on the menu and allow the user to select --source to FRED.

  • Is it normal that the high/low is the same? And that sometimes high is bigger than last_close and others low is smaller?

  • Instead of the title being "Country yield curve" can we have the country name, e.g. "Portugal yield curve". That way you can remove the first column with the country which doesn't add much since it's always the same.

  • Can we follow your suggestion to show the last_close graph (similar to current yield) and if user does --raw or -r we display the table?

  • Let's add the countries to autocomplete, similar to these: self.choices["index"] = {c: None for c in yfinance_model.INDICES}

Screenshot 2022-04-24 at 18 11 08

EXTRA:

This is a bit more advanced and we can go through it together, but we can improve the way to parse the country by not necessarily needing the underscore.

Regarding this argument parsing:

parser.add_argument(
           "-c",
           "--country",
           action="store",
           dest="country",
           type=str,
           choices=investingcom_model.countries,
           default="united_states",
           help="Display yield curve for specific country. (spaces: '_')",
       )

We can have a smarter solution for 2 arguments, instead of the underscore. This is what I suggest:

parser.add_argument(
           "-c",
           "--country",
           action="store",
           dest="country",
           type=check_correct_country,
           nargs="+",
           default="United States",
           help="Display yield curve for specific country. ",
       )

And then you can create a check_correct_country function that looks like:

def check_correct_country(country: str) -> str:
    """Argparse type to check that correct country is inserted"""
    if country not in investpy.bonds.get_bond_countries():
        log_and_raise(
             argparse.ArgumentTypeError(f"{country} is an invalid country. Choose from {', '.join(investpy.bonds.get_bond_countries())}")
        )
    return country

@DidierRLopes
Copy link
Collaborator

I like your last suggestion as well, for us to start a bonds menu 😄

@deeleeramone
Copy link
Contributor

This is really sexy. Looking forward to this! Nice work!!

@DidierRLopes DidierRLopes added the feat XS Extra small feature label Apr 24, 2022
@DidierRLopes
Copy link
Collaborator

Also, the main issue you are having is because you are on Windows and need to change the ending line.

Not sure if you did this but you can run pre-install commit so that it checks most of our linting/formatting checks on your laptop locally before being checked on remote

@weisberg
Copy link

I like your last suggestion as well, for us to start a bonds menu 😄

I suggest that we use fixed income instead of bonds as a high-level menu name because the market is very broad.

@montezdesousa
Copy link
Contributor Author

I like your last suggestion as well, for us to start a bonds menu 😄

I suggest that we use fixed income instead of bonds as a high-level menu name because the market is very broad.

I like your last suggestion as well, for us to start a bonds menu 😄

I suggest that we use fixed income instead of bonds as a high-level menu name because the market is very broad.

Yes you are right, that makes sense

@jmaslek
Copy link
Collaborator

jmaslek commented Apr 25, 2022

This is awesome! I think @DidierRLopes hit a bunch of good points. My main comment is that I think short abbreviated are more suited for the terminal, so I would do as Didier mentioned and put this all into the yield function, and potentially rename that to ycrv

@montezdesousa
Copy link
Contributor Author

NICE WORK!! 🚀

Here is my review:

  • Instead of having a new command let's put this under yield and use this source by default as the one that is currently available relies on FRED.
  • Ok, will take @jmaslek tip and make it ycrv instead
  • Let's put then Source: Investpy/FRED on the menu and allow the user to select --source to FRED.
  • Ok
  • Is it normal that the high/low is the same? And that sometimes high is bigger than last_close and others low is smaller?
  • It should be very rare. I see that it only occurs in some cases (like the US). Increasing to 3 or 4 decimal places should help when the number is actually different. But I'll dig on the others.
  • Instead of the title being "Country yield curve" can we have the country name, e.g. "Portugal yield curve". That way you can remove the first column with the country which doesn't add much since it's always the same.
  • Sure
  • Can we follow your suggestion to show the last_close graph (similar to current yield) and if user does --raw or -r we display the table?
  • So graph is the default?
  • Let's add the countries to autocomplete, similar to these: self.choices["index"] = {c: None for c in yfinance_model.INDICES}
  • Ok
Screenshot 2022-04-24 at 18 11 08

EXTRA:

  • ok will try to improve this

This is a bit more advanced and we can go through it together, but we can improve the way to parse the country by not necessarily needing the underscore.

Regarding this argument parsing:

parser.add_argument(
           "-c",
           "--country",
           action="store",
           dest="country",
           type=str,
           choices=investingcom_model.countries,
           default="united_states",
           help="Display yield curve for specific country. (spaces: '_')",
       )

We can have a smarter solution for 2 arguments, instead of the underscore. This is what I suggest:

parser.add_argument(
           "-c",
           "--country",
           action="store",
           dest="country",
           type=check_correct_country,
           nargs="+",
           default="United States",
           help="Display yield curve for specific country. ",
       )

And then you can create a check_correct_country function that looks like:

def check_correct_country(country: str) -> str:
    """Argparse type to check that correct country is inserted"""
    if country not in investpy.bonds.get_bond_countries():
        log_and_raise(
             argparse.ArgumentTypeError(f"{country} is an invalid country. Choose from {', '.join(investpy.bonds.get_bond_countries())}")
        )
    return country

@JerBouma
Copy link
Contributor

JerBouma commented May 4, 2022

Hi @montezdesousa, when do you expect to have time for this and/or do you need any help?

@montezdesousa
Copy link
Contributor Author

I'll push during this week and get back if need help @JerBouma

@@ -82,7 +84,7 @@
API_TWITTER_BEARER_TOKEN = os.getenv("OPENBB_API_TWITTER_BEARER_TOKEN") or "REPLACE_ME"

# https://fred.stlouisfed.org/docs/api/api_key.html
API_FRED_KEY = os.getenv("OPENBB_API_FRED_KEY") or "REPLACE_ME"
Copy link
Collaborator

Choose a reason for hiding this comment

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

@montezdesousa you want to remove your key here 😄

"--source",
action="store",
dest="source",
type=str,
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can use parameter choices here, to only allow investpy or fred

@DidierRLopes
Copy link
Collaborator

Hey @montezdesousa do you think you would have time to fix the tests here so we can merge it? I think is the only thing missing

@montezdesousa
Copy link
Contributor Author

Hey @montezdesousa do you think you would have time to fix the tests here so we can merge it? I think is the only thing missing

Yes sorry it's taking a bit longer than expected, but will try to find some time during the weekend to finish the tests to merge asap

@DidierRLopes
Copy link
Collaborator

Hey @montezdesousa do you think you would have time to fix the tests here so we can merge it? I think is the only thing missing

Yes sorry it's taking a bit longer than expected, but will try to find some time during the weekend to finish the tests to merge asap

np, thanks @montezdesousa !

@jmaslek
Copy link
Collaborator

jmaslek commented May 17, 2022

Yes sorry it's taking a bit longer than expected, but will try to find some time during the weekend to finish the tests to merge asap

Hey! I just saw it was one test so I pushed the updated .txt file (just needed to run pytest tests/openbb_terminal/economy/test_economy_controller.py --rewrite-expected). Im happy if you want to mark as ready

@DidierRLopes DidierRLopes marked this pull request as ready for review May 17, 2022 22:51
@DidierRLopes DidierRLopes self-requested a review May 17, 2022 22:51
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.

LETSGOOOO 🚀

@DidierRLopes DidierRLopes merged commit 4f692f2 into OpenBB-finance:main May 17, 2022
@montezdesousa
Copy link
Contributor Author

Yes sorry it's taking a bit longer than expected, but will try to find some time during the weekend to finish the tests to merge asap

Hey! I just saw it was one test so I pushed the updated .txt file (just needed to run pytest tests/openbb_terminal/economy/test_economy_controller.py --rewrite-expected). Im happy if you want to mark as ready

Thanks!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat XS Extra small feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants