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

Added more file formats for compression #1597

Merged
merged 27 commits into from
Sep 21, 2022

Conversation

aadityasinha-dotcom
Copy link
Contributor

@aadityasinha-dotcom aadityasinha-dotcom commented Apr 15, 2022

🚀 🚀 Pull Request

Checklist:

  • My code follows the style guidelines of this project and the Contributing document
  • I have commented my code, particularly in hard-to-understand areas
  • I have kept the coverage-rate up
  • I have performed a self-review of my own code and resolved any problems
  • I have checked to ensure there aren't any other open Pull Requests for the same change
  • I have described and made corresponding changes to the relevant documentation
  • New and existing unit tests pass locally with my changes

Changes

  1. I have added more image file formats for compression.
  2. I have also added read-only formats for compression.

@aadityasinha-dotcom
Copy link
Contributor Author

@farizrahman4u can you review the changes?

Copy link
Contributor

@farizrahman4u farizrahman4u left a comment

Choose a reason for hiding this comment

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

Need unit tests.

hub/compression.py Outdated Show resolved Hide resolved
hub/compression.py Show resolved Hide resolved
hub/compression.py Outdated Show resolved Hide resolved
hub/compression.py Outdated Show resolved Hide resolved
hub/compression.py Outdated Show resolved Hide resolved
hub/compression.py Outdated Show resolved Hide resolved
@aadityasinha-dotcom
Copy link
Contributor Author

aadityasinha-dotcom commented Apr 20, 2022

@farizrahman4u Can you review the changes I have made?

Copy link
Contributor

@farizrahman4u farizrahman4u left a comment

Choose a reason for hiding this comment

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

Remove the files not required from the PR, address ALL comments and lint.

hub/compression.py Outdated Show resolved Hide resolved
hub/compression.py Outdated Show resolved Hide resolved
hub/core/tests/test_compression.py Outdated Show resolved Hide resolved
Copy link
Contributor

@farizrahman4u farizrahman4u left a comment

Choose a reason for hiding this comment

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

The test images are in the wrong location, they should be in hub/tests/dumm_data/images, and their paths should be returned from compressed_image_paths.

hub/core/tests/test_compression.py Outdated Show resolved Hide resolved
hub/compression.py Outdated Show resolved Hide resolved
aadityasinha-dotcom and others added 3 commits May 6, 2022 17:57
Co-authored-by: Fariz Rahman <farizrahman4u@gmail.com>
Co-authored-by: Fariz Rahman <farizrahman4u@gmail.com>
Copy link
Contributor

@farizrahman4u farizrahman4u left a comment

Choose a reason for hiding this comment

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

Remove the Pillow and hub-test-resources folders from the PR.

hub/core/tests/test_compression.py Outdated Show resolved Hide resolved
@aadityasinha-dotcom
Copy link
Contributor Author

@farizrahman4u Can you review the changes?

hub/core/tests/test_compression.py Outdated Show resolved Hide resolved
@farizrahman4u farizrahman4u added the trigger-test Label trigger to run tests on PRs label Jul 19, 2022
@activeloop-bot activeloop-bot removed the trigger-test Label trigger to run tests on PRs label Jul 19, 2022
@aadityasinha-dotcom
Copy link
Contributor Author

@farizrahman4u any update?

hub/compression.py Outdated Show resolved Hide resolved
hub/api/tests/test_api.py Outdated Show resolved Hide resolved
hub/core/tests/test_compression.py Outdated Show resolved Hide resolved
hub/api/tests/test_api.py Show resolved Hide resolved
@tatevikh tatevikh added the trigger-test Label trigger to run tests on PRs label Aug 23, 2022
@activeloop-bot activeloop-bot removed the trigger-test Label trigger to run tests on PRs label Aug 23, 2022
@FayazRahman FayazRahman added the trigger-test Label trigger to run tests on PRs label Aug 25, 2022
@activeloop-bot activeloop-bot removed the trigger-test Label trigger to run tests on PRs label Aug 25, 2022
@tatevikh
Copy link
Collaborator

@aadityasinha-dotcom tests are still failing. This needs to be fixed, otherwise we cannot merge. Thank you!

@aadityasinha-dotcom
Copy link
Contributor Author

@aadityasinha-dotcom tests are still failing. This needs to be fixed, otherwise we cannot merge. Thank you!

Yes I will look into it.

@aadityasinha-dotcom aadityasinha-dotcom requested review from FayazRahman and removed request for farizrahman4u September 14, 2022 03:22
@FayazRahman FayazRahman added the trigger-test Label trigger to run tests on PRs label Sep 16, 2022
@activeloop-bot activeloop-bot removed the trigger-test Label trigger to run tests on PRs label Sep 16, 2022
@aadityasinha-dotcom
Copy link
Contributor Author

Hey @FayazRahman , why these tests are failing I tried to run test_tensorflow.py in my system but it was giving no errors.

@FayazRahman
Copy link
Contributor

@aadityasinha-dotcom Can you pull main and try again?

@aadityasinha-dotcom
Copy link
Contributor Author

@FayazRahman I have pulled the main branch.

@tatevikh tatevikh added the trigger-test Label trigger to run tests on PRs label Sep 20, 2022
@activeloop-bot activeloop-bot removed the trigger-test Label trigger to run tests on PRs label Sep 20, 2022
@codecov
Copy link

codecov bot commented Sep 21, 2022

Codecov Report

Base: 91.98% // Head: 92.21% // Increases project coverage by +0.22% 🎉

Coverage data is based on head (a6988ea) compared to base (d4f625f).
Patch coverage: 76.92% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1597      +/-   ##
==========================================
+ Coverage   91.98%   92.21%   +0.22%     
==========================================
  Files         245      246       +1     
  Lines       25689    26104     +415     
==========================================
+ Hits        23631    24072     +441     
+ Misses       2058     2032      -26     
Flag Coverage Δ
unittests 92.21% <76.92%> (+0.22%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
hub/api/tests/test_api.py 99.78% <ø> (+1.04%) ⬆️
hub/tests/path_fixtures.py 90.72% <ø> (+3.17%) ⬆️
hub/core/tests/test_readonly.py 64.70% <64.70%> (ø)
hub/compression.py 95.91% <100.00%> (-1.81%) ⬇️
hub/core/tests/test_compression.py 68.29% <100.00%> (-25.57%) ⬇️
hub/tests/common.py 91.04% <0.00%> (-5.98%) ⬇️
hub/core/compression.py 80.80% <0.00%> (-0.83%) ⬇️
hub/integrations/wandb/wandb.py 48.05% <0.00%> (-0.72%) ⬇️
... and 80 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tatevikh tatevikh merged commit 2c92f60 into activeloopai:main Sep 21, 2022
@mikayelh
Copy link
Collaborator

Yaaaaay, @aadityasinha-dotcom - congrats on the contribution! Can you please hit me up in the community slack to arrange for some swag? :)

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.

7 participants