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

ECI Data #4

Closed
wants to merge 19 commits into from
Closed

ECI Data #4

wants to merge 19 commits into from

Conversation

akshaya-seetharam
Copy link
Contributor

To add csv files, process_pdf script, and model for ECI dataset into the main branch

Copy link
Contributor

@czheng10 czheng10 left a comment

Choose a reason for hiding this comment

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

looks good to me!

backend/app/models.py Outdated Show resolved Hide resolved
Copy link
Member

@JusticeV452 JusticeV452 left a comment

Choose a reason for hiding this comment

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

Thanks for getting this in! It will be very useful to have a standardized way of processing pdfs if we need to add more later.

There are a few changes you should make before we merge this into main:

  • There are still some checks that are failing
  • I was not able to run the code as is, however, due to the way file paths are checked (more detail in one of my comments)

Also, in the future, could you

  • include how the PR will affect main in the title (something like "Add ECI pdf data and pdf parser")
  • add me as a reviewer so I get notified of the new PR
  • assign everyone who worked on the branch so they are notified when I add comments

One other thing: If you have time before the hack session, could you add an api_view and code to update the database like in #2 so you can start working with the data on the front end?

backend/app/process_pdfs.py Show resolved Hide resolved
backend/app/process_pdfs.py Show resolved Hide resolved
@czheng10 czheng10 closed this Mar 19, 2024
@czheng10 czheng10 deleted the eci-data branch March 19, 2024 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants