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

Fill missing static data (sector, industry, country and region) on portfolio creation #2072

Merged
merged 27 commits into from
Jul 18, 2022

Conversation

montezdesousa
Copy link
Contributor

@montezdesousa montezdesousa commented Jul 12, 2022

Description

This PR populates the orderbook loaded in the portfolio menu with sector, industry, country and region if they are not provided by user. This will be useful to allow integration with broker accounts that just provide transaction data (e.g. price, quantity, ticker...) and not much static data. There's a PR already trying to start this integration by exporting a formatted transaction file. See #2065 but it was being blocked by this issue.

Post-review changes:

  • Save company data locally after first download, this should speed up load orderbook process by avoiding unnecessary downloads after the first time

  • Add progress bar update on loading checkpoints

  • Avoid rewriting when company data provided by user

  • Summary of the change / bug fix.

  • Relevant motivation and context.

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

So, when I don't fill anything in the Orderbook, but do keep the columns it doesn't work. Should we remove those altogether? I think we should so there is less manual input required.

image

If we do, however, it takes longer to load. Could you add in a progression bar or, otherwise, an option to export to .csv so it loads quicker?

@JerBouma
Copy link
Contributor

@montezdesousa How far are you with this PR?

@montezdesousa
Copy link
Contributor Author

@montezdesousa How far are you with this PR?

I'll be on it this afternoon

@JerBouma
Copy link
Contributor

Is it ready?

@montezdesousa
Copy link
Contributor Author

montezdesousa commented Jul 15, 2022

Is it ready?

yes, after this one passes I have a WIP (#2091) to change the degiro export orderbook path to portfolio/holdings and the user should be able to paexport their orderbook in degiro and then access the file in the load command of portfolio menu

@DidierRLopes
Copy link
Collaborator

@JerBouma is this good to be merged?

@montezdesousa
Copy link
Contributor Author

montezdesousa commented Jul 18, 2022

@JerBouma is this good to be merged?

@JerBouma as I mentioned to you, my implementation of returns was very poor performance wise, so I added a new commit that I believe fixes this (if ok calculating returns went from 25s to 0.6s for the Public_Equity_Orderbook.xlsx) and the progress bar does not need to be that long

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.

Sounds great!

@montezdesousa montezdesousa removed the request for review from Chavithra July 18, 2022 17:19
@montezdesousa montezdesousa merged commit ec9a88e into main Jul 18, 2022
@montezdesousa montezdesousa deleted the import_portfolio branch July 18, 2022 17:51
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.

4 participants