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.

Sign up for GitHub

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

Jump to bottom

feat(sec): add fiscal period filter for SEC company concepts #6685

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from
Draft

feat(sec): add fiscal period filter for SEC company concepts #6685

wants to merge 4 commits into from

Conversation

dijonkitchen
Copy link

@dijonkitchen dijonkitchen commented Sep 22, 2024 •
edited
Loading

Pull Request the OpenBB Platform

Description

Resolves part of #6654 (comment)

Allows for SEC company concepts to be filtered by fiscal period.

  • 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?

  • Please describe the tests that you ran to verify your changes.

  • Please provide instructions so we can reproduce.

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

  • Ensure all unit and integration tests pass.

  • If you modified/added command(s):

    • Ensure the command(s) execute with the expected output.
      • API.
      • Python Interface.
    • If applicable, please add new tests for the command (see CONTRIBUTING.md to leverage semi-automated testing).

- If a new provider was introduced or a new fetcher was added to an existing provider: - [ ] Ensure the existing tests pass. - [ ] Ensure the new provider and/or fetcher is stable and usable. - [ ] If applicable, please add new tests for the provider and/or fetcher (see [CONTRIBUTING.md](/openbb_platform/CONTRIBUTING.md) to leverage semi-automated testing). - If a new provider or extension was added: - [ ] Update the list of [Extensions](/openbb_platform/EXTENSIONS.md). - [ ] Update the list of [Providers](/openbb_platform/PROVIDERS.md). - [ ] If it's a community extension or provider, update the [integration tests GitHub Action workflow](/.github/workflows/platform-api-integration-test.yml).

Checklist

  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have adhered to the GitFlow naming convention and my branch name is in the format of feature/feature-name or hotfix/hotfix-name.
  • I ensure that I am following the CONTRIBUTING guidelines.

@CLAassistant
Copy link

CLAassistant commented Sep 22, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@dijonkitchen dijonkitchen mentioned this pull request Sep 22, 2024
Open
deeleeramone
deeleeramone requested changes Sep 24, 2024
View reviewed changes
Copy link
Contributor

@deeleeramone deeleeramone left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I know you have this marked as a draft, so forgive me for jumping in early.

The main issue, and why I excluded filtering by a fiscal quarter, is that Q4 is not a reliable piece of data. A large portion of Facts do not report Q4, and relying on CY/frame field is also not really reliable for this purpose. I had initially made an attempt to cover this, but needed to withdraw that functionality in the face of inconsistent quality of results.

Screenshot 2024-09-23 at 7 49 08 PM

If you were able to devise a parsing method that accounted for Q4 across all Facts, and filtered out the duplicate entries so that there was only one valid entry with values representing the change Q/Q for items that are not point-in-time, we'd start to have a fighting chance at making quarterly statements that are comparable over time.

@deeleeramone deeleeramone added the enhancement Enhancement label Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deeleeramone deeleeramone deeleeramone requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
enhancement Enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dijonkitchen @CLAassistant @deeleeramone