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 Economy/Plot - Does not read stored datasets properly #2112

Merged
merged 13 commits into from
Jul 20, 2022
Merged

Conversation

montezdesousa
Copy link
Contributor

@montezdesousa montezdesousa commented Jul 19, 2022

Description

This PR fixes #2089. The current help menu is not updating the stored datasets in at least two scenarios.

  1. When users provide more than 1 series to display/store it just saves the name of the first, despite storing every series.
  • 1 series name on menu, 2 or more stored, for each command used
  1. When users provide 1 series from a specific command (e.g. index) that was used before. In this case, the code is substituting the new series for the previous, but the help menu is appending the names.
  • 2 or more series names on menu, 1 stored, for each command used

To solve this I added an helper method that builds the string at each macro, treasury, fred and index calls.

Additionally, to keep the user updated the help menu is refreshed so print_help() was added after each one of the four command calls if feature flag for help on exit is on. Currently, 1 series at a time just the last one is stored. If user wants to have both he must load both at the same time, for example: index dxy jp_n225.

The help description was updated in terminal and website to exclude -st flag, apparently it is not used anymore to store datasets.

  • Summary of the change / bug fix.
  • Link # issue, if applicable.
  • Relevant motivation and context.

How has this been tested?

  • Load datasets
    macro -p RGDP RPUC
    treasury 5y 10y
    fred -p gdp CORESTICKM159SFRBATL
    index dxy jp_n225
  • Plot datasets
    plot --y1 dxy --y2 jp_n225
    plot --y1 Nominal_10-year --y2 United_States_RGDP

Checklist:

Others

  • I have performed a self-review of my own code.
  • 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/....

@DidierRLopes DidierRLopes requested a review from JerBouma July 19, 2022 15:39
@montezdesousa montezdesousa added the bug Fix bug label Jul 19, 2022
@montezdesousa montezdesousa marked this pull request as ready for review July 19, 2022 15:48
Copy link
Contributor

@JerBouma JerBouma left a comment

Choose a reason for hiding this comment

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

Why are we not saving the names on one line? E.g.

│ Stored datasets:                                                                                                                                                                │
│   index    : dxy                                                                                                                                                                     │
│   index    : jp_n225                                                                                                                                                             │

Becomes:

│ Stored datasets:                                                                                                                                                                     │
│   index    : dxy, jp_n225                                                                                                                                                          │

Makes it a lot more tidier. Other than that, all good with the changes.

@montezdesousa
Copy link
Contributor Author

montezdesousa commented Jul 19, 2022

Why are we not saving the names on one line? E.g.

│ Stored datasets:                                                                                                                                                                │
│   index    : dxy                                                                                                                                                                     │
│   index    : jp_n225                                                                                                                                                             │

Becomes:

│ Stored datasets:                                                                                                                                                                     │
│   index    : dxy, jp_n225                                                                                                                                                          │

Makes it a lot more tidier. Other than that, all good with the changes.

don't know why, but that actually makes sense yes, pls check if it's ok now

Copy link
Contributor

@JerBouma JerBouma left a comment

Choose a reason for hiding this comment

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

Seems to be weirdly indented and now doesn't correctly understand when I plot two indices seperately.

image

I would expect:

  • Whether I plot them separately or together, they appear on the same line.
  • It isn't indented this much.

@montezdesousa
Copy link
Contributor Author

Seems to be weirdly indented and now doesn't correctly understand when I plot two indices seperately.

image

I would expect:

  • Whether I plot them separately or together, they appear on the same line.
  • It isn't indented this much.

It's aligned by the colon of "Stored datasets", but I can pull it a bit back to match previous indent.

I believe that's how the feature is designed, the stored datasets for a specific command are reset at each call. We could change it, but that's how I see it working. The bug was that the help menu was not actually displaying what was really stored, it was appending data set names to the menu string, it was not consistent with actual stored data.

For example if we follow this command sequence:
index sp400 sp500 -> store sp400 and sp500
index jp_n225 -> store jp_n225 (and forget sp400 and sp500)

So, the only available dataset for plot would be the last jp_n225

@JerBouma
Copy link
Contributor

So I'd find that a bit strange. E.g. let's say I am looking at unemployment and the S&P 500. Then I decide.. but wait, what if I compare unemployment with inflation? Now I need to call both unemployment and inflation together to not lose my plot with unemployment.

I feel like it handicaps the user a little bit.

@montezdesousa
Copy link
Contributor Author

So I'd find that a bit strange. E.g. let's say I am looking at unemployment and the S&P 500. Then I decide.. but wait, what if I compare unemployment with inflation? Now I need to call both unemployment and inflation together to not lose my plot with unemployment.

I feel like it handicaps the user a little bit.

agreed, I'll try to change that to append datasets as well then

Copy link
Contributor

@JerBouma JerBouma left a comment

Choose a reason for hiding this comment

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

Details:

  • All of the options appear even if I didn't fill in any. Doesn't make much sense to me.
  • It seems that if I input index sp500,sp400 it will return index: crypto200,sp500 while if I add in another index sp400 it will write it out as index: crypto200,sp500, sp400. Missing a space!

image

Copy link
Contributor

@JerBouma JerBouma left a comment

Choose a reason for hiding this comment

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

Looking good! Well done :)

@montezdesousa montezdesousa merged commit ca45410 into main Jul 20, 2022
@montezdesousa montezdesousa deleted the issue2089 branch July 20, 2022 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fix bug
Projects
None yet
2 participants