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

Feature: CSV parsing for hub.auto #711

Merged
merged 14 commits into from
Mar 25, 2021
Merged

Feature: CSV parsing for hub.auto #711

merged 14 commits into from
Mar 25, 2021

Conversation

dhiganthrao
Copy link
Contributor

The purpose of this PR is to add code for auto parsing of CSV files to hub.auto. Kindly go through it and let me know where I can improve!

@github-actions
Copy link

Locust summary

Git references

Initial: e3f1c84
Terminal: f2d7ae4
hub/auto/tabular/csv.py
Changes:
hub/auto/infer.py
Changes:
  • Name: _find_root
    Type: function
    Changed lines: 2
    Total lines: 31
    hub/auto/tests/test_tabular_data.py
    Changes:

    @dhiganthrao
    Copy link
    Contributor Author

    Apologies for the lint error. Will rectify it ASAP

    @mynameisvinn
    Copy link
    Contributor

    copying @McCrearyD

    @verbose-void verbose-void self-assigned this Mar 23, 2021
    @verbose-void verbose-void added the enhancement New feature or request label Mar 23, 2021
    @verbose-void
    Copy link
    Contributor

    Apologies for the lint error. Will rectify it ASAP

    no need to apologize, lint errors happen :) -- you can refer to the contributing doc for black setup with vscode.

    verbose-void
    verbose-void suggested changes Mar 23, 2021
    View reviewed changes
    Copy link
    Contributor

    @verbose-void verbose-void left a comment

    Choose a reason for hiding this comment

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

    i made some minor requests, after those are done should be ready to merge :P

    verbose-void
    verbose-void reviewed Mar 23, 2021
    View reviewed changes
    hub/auto/infer.py Outdated Show resolved Hide resolved
    verbose-void
    verbose-void suggested changes Mar 23, 2021
    View reviewed changes
    Copy link
    Contributor

    @verbose-void verbose-void 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.

    last changes

    @dhiganthrao
    Copy link
    Contributor Author

    @McCrearyD pushed the changes you've requested! Re-requesting review from you.

    verbose-void
    verbose-void approved these changes Mar 24, 2021
    View reviewed changes
    Copy link
    Contributor

    @verbose-void verbose-void left a comment

    Choose a reason for hiding this comment

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

    perfect!

    @verbose-void
    Copy link
    Contributor

    CLCI was failing tests, but those were dependency issues. addressed those & got them passing. awaiting CLCI run, then merging

    verbose-void
    verbose-void suggested changes Mar 24, 2021
    View reviewed changes
    Copy link
    Contributor

    @verbose-void verbose-void left a comment

    Choose a reason for hiding this comment

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

    my previous approval was wrong, please do these changes

    @dhiganthrao
    Copy link
    Contributor Author

    dhiganthrao commented Mar 25, 2021 •
    edited
    Loading

    @McCrearyD made some changes with respect to your last review. Hope this is fine!

    Detailing the changes I've made:

    • I've added new csv files with different and random values, to check if the parser still works properly on it. Please let me know if I should change the csv files if they are unsatisfactory.
    • I'm now reading a Pandas Dataframe from the csv file directly to compare it with the dataset generated by hub.auto. This makes it easier for me to check whether the column names and their data types are the same and so on.
    • I've also written different test conditions to implement what you've asked me to do in your last review.

    Please go through it and let me know where I can improve.

    verbose-void
    verbose-void approved these changes Mar 25, 2021
    View reviewed changes
    Copy link
    Contributor

    @verbose-void verbose-void left a comment

    Choose a reason for hiding this comment

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

    this is perfect. stellar work!

    @verbose-void verbose-void merged commit 42f6371 into activeloopai:master Mar 25, 2021
    @dhiganthrao dhiganthrao deleted the feature/auto-csv branch March 28, 2021 17:24
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @verbose-void verbose-void verbose-void approved these changes

    Assignees

    @verbose-void verbose-void

    @dhiganthrao dhiganthrao

    Labels
    enhancement New feature or request
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.
    3 participants
    @dhiganthrao @mynameisvinn @verbose-void