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 optional columns before compute with columns. #2445

Merged

Conversation

jfarid27
Copy link
Contributor

@jfarid27 jfarid27 commented Aug 28, 2022

Description

Add the preprocessing step of portfolio columns before using the columns. This PR moves the optional column filling earlier into the stack so one can add a portfolio with only the basic required columns.

Issues

#2444

Motivation

Trying to add a custom overwrite into Portfolio requires users to exactly match existing columns. This is slightly annoying since some users may have their own dataset transformers or sheets, and they want to pull only the required columns. This PR helps with that.

  • Summary of the change / bug fix.
  • Link # issue, if applicable.
  • Screenshot of the feature or the bug before/after fix, if applicable.
  • Relevant motivation and context.
  • List any dependencies that are required for this change.

How has this been tested?

Ran pytests and cleared.

  • Please describe the tests that you ran to verify your changes.
  • Provide instructions so we can reproduce.
  • Please also list any relevant details for your test configuration.

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/....

@DidierRLopes DidierRLopes added the portfolio Portfolio menu label Aug 28, 2022
Copy link
Contributor

@montezdesousa montezdesousa left a comment

Choose a reason for hiding this comment

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

Hey! Thanks for the contribution, seems to work fine for me and solves the bug.

The approach is good, later we should add a similar check for required_fields : Date, Ticker, Type, Price, Quantity, Side and warn the user they must be there stopping pre-processing. Now it is continuing the workflow and found this case where it only stops in " Loading price data: Error: "['Type', 'Investment'] not in index" " and 'Investment' is not even required.

@colin99d colin99d merged commit bfb14e1 into OpenBB-finance:main Sep 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
portfolio Portfolio menu
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants