Changed str return to include Tensor-Wise information for issue #1439. #1543

neel2299 · 2022-03-15T06:47:42Z

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

Used pandas to_string method to get the tensor information into a table.

CLAassistant · 2022-03-15T06:47:50Z

All committers have signed the CLA.

farizrahman4u

This PR reverts hidden tensors and can't be accepted in the current state.

hub/core/dataset/dataset.py

neel2299 · 2022-03-15T17:36:12Z

@farizrahman4u Hello!!
I made the logic this time, please check it out...

neel2299 · 2022-03-15T17:41:38Z

This PR reverts hidden tensors and can't be accepted in the current state.

For some reason, I noticed this comment only now, Can you please point me to the documentation for hidden tensors? I didn't know that my commit was affecting something.

farizrahman4u

I have commented on the lines which reverts previous PRs. Seems your git history is messed up.

hub/core/dataset/dataset.py

done it again

neel2299 · 2022-03-17T09:24:44Z

Hello!!, I have done it afresh, and checked the comparison as well. This time its showing only the lines I have added and removed.(Previously I think I made a mess while trying to manually compare the files with the pip version.)

farizrahman4u

Move the helper methods (including the logic in __str__ to a util file, like hub/util/summary.py
Use snake case everywhere (except class names)
Instead of changing __str__, add .summary() method to both Tensor and Dataset.

Master

neel2299 · 2022-03-27T17:08:32Z

@farizrahman4u Thank you for the review!! I have made the changes requested and added compression type in the table. Please tell me if it needs other changes.

FayazRahman · 2022-03-28T05:01:43Z

hub/core/dataset/dataset.py

+            tensor_name = tensor
+            tensor_htype = tensor_object.htype
+            tensor_shape = str(tensor_object.shape)
+            tensor_dtype = tensor_object.dtype.name


tensor_object.dtype could be None and it will not have name attribute. Just do ds.create_tensor("abc") to reproduce the error.

I made the changes. Can you please review it once again? @FayazRahman @farizrahman4u @mikayelh @tatevikh

Handling None Types

neel2299 · 2022-03-28T14:45:43Z

@FayazRahman Thank You for the review!! I have added support for None types for all attributes except for tensor_shape (It initializes at (0,)).

Also while trying to add new tensors I noticed that with ds.create_tensor("abc") , along with "abc" we get another tensor "_abc_id" . This was happening with the current main hub repo as well. Is it getting created for some specific reason?

farizrahman4u

Use self.tensors instead of self.version_state["full_tensors"] to hide hidden tensors.
Many variables are still in camel case, please switch to snake case as mentioned in the previous review.
A simple unit test would be nice.
Move the summary logic for dataset and tensor to a util file, like hub/util/summary.py (Again, from previous review).

farizrahman4u · 2022-04-03T12:23:36Z

hub/core/dataset/dataset.py

+        divider = ["-------"] * 5
+        tensor_dict = self.version_state[
+            "full_tensors"
+        ]  # Creating a list of tensors in the dataset


Comment not required - this is a dict not a list, and should self.tensors instead of self.version_state["full_tensors"]

Master

neel2299 · 2022-04-05T04:50:37Z

Hello!! Thanks alot for the review. I have made the changes. Last time I actually thought that we had to move only the helper functions out to utils(not the ones which were implemented as methods). Sorry about the late reply, had my end terms going. I will write unit tests for both tensor and dataset str methods soon and if its alright in another PR. Please check out the new changes. @farizrahman4u @FayazRahman

farizrahman4u · 2022-04-05T13:30:50Z

Hello!! Thanks alot for the review. I have made the changes. Last time I actually thought that we had to move only the helper functions out to utils(not the ones which were implemented as methods). Sorry about the late reply, had my end terms going. I will write unit tests for both tensor and dataset str methods soon and if its alright in another PR. Please check out the new changes. @farizrahman4u @FayazRahman

Tests need to in the same PR @neel2299 . Just calling .summary on tensors and datasets with htype specified and unspecified, compression specified and unspecified etc would do.

adding tests

summary test

neel2299 · 2022-04-06T06:59:07Z

Thanks for the review once again :) . I changed the tests for test_stringify according to the new implementation and added summary tests. @farizrahman4u @FayazRahman

removing unneeded import

added test for local path

neel2299 · 2022-04-06T20:13:44Z

Added test for local path (the failed test) @farizrahman4u @FayazRahman

rebase

Merge pull request #15 from neel2299/main

tatevikh · 2022-04-07T15:19:03Z

Great job @neel2299. Thanks for your contribution to Hub 🚀 .

neel2299 · 2022-04-08T06:56:56Z

Learned alot!! @tatevikh . Thanks to community :)

farizrahman4u suggested changes Mar 15, 2022

View reviewed changes

hub/core/dataset/dataset.py Outdated Show resolved Hide resolved

tatevikh requested a review from FayazRahman March 15, 2022 14:29

farizrahman4u suggested changes Mar 16, 2022

View reviewed changes

neel2299 closed this Mar 16, 2022

neel2299 force-pushed the main branch from 678af35 to 225179d Compare March 16, 2022 18:14

neel2299 and others added 2 commits March 17, 2022 14:33

done it again

9705ea1

Merge pull request #6 from neel2299/master

6745bc8

done it again

neel2299 reopened this Mar 17, 2022

neel2299 mentioned this pull request Mar 19, 2022

[Feature] pretty prints of objects #1439

Closed

2 tasks

neel2299 added 2 commits March 21, 2022 02:28

attribute name

a0665fe

attribute name

c3c5547

farizrahman4u suggested changes Mar 23, 2022

View reviewed changes

neel2299 and others added 3 commits March 27, 2022 22:07

making the changes requested

ab3fcc8

black formatting

bb1f198

Merge pull request #8 from neel2299/master

63b1a53

Master

Merge branch 'main' into main

1ae1508

FayazRahman suggested changes Mar 28, 2022

View reviewed changes

neel2299 and others added 2 commits March 28, 2022 19:37

Handling None Types

35a2576

Merge pull request #9 from neel2299/master

72619c0

Handling None Types

farizrahman4u suggested changes Apr 3, 2022

View reviewed changes

farizrahman4u reviewed Apr 3, 2022

View reviewed changes

tatevikh requested a review from FayazRahman April 4, 2022 17:24

neel2299 added 2 commits April 5, 2022 09:04

made requested changes

bf3f517

minor changes

69a3161

neel2299 and others added 3 commits April 5, 2022 09:47

black

263a63e

Merge pull request #10 from neel2299/master

f4f87c8

Master

Merge branch 'main' into main

f42d44a

tatevikh requested a review from farizrahman4u April 5, 2022 13:22

neel2299 and others added 6 commits April 6, 2022 10:27

adding tests

b20838f

black

05970ae

removing comments

493360c

Merge pull request #11 from neel2299/master

dda48c9

adding tests

summary test

e4eba90

Merge pull request #12 from neel2299/master

4acf25a

summary test

neel2299 and others added 2 commits April 6, 2022 12:35

removing unneeded import

c4cf2be

Merge pull request #13 from neel2299/master

cf9b1a0

removing unneeded import

farizrahman4u approved these changes Apr 6, 2022

View reviewed changes

farizrahman4u added the trigger-test Label trigger to run tests on PRs label Apr 6, 2022

activeloop-bot removed the trigger-test Label trigger to run tests on PRs label Apr 6, 2022

neel2299 and others added 2 commits April 7, 2022 01:31

added test for local path

d0f43ce

Merge pull request #14 from neel2299/master

f446314

added test for local path

FayazRahman approved these changes Apr 7, 2022

View reviewed changes

FayazRahman added the trigger-test Label trigger to run tests on PRs label Apr 7, 2022

activeloop-bot removed the trigger-test Label trigger to run tests on PRs label Apr 7, 2022

neel2299 added 2 commits April 7, 2022 15:18

Merge pull request #15 from neel2299/main

2b3bd41

rebase

Merge pull request #16 from neel2299/master

ec999a6

Merge pull request #15 from neel2299/main

tatevikh merged commit 9fd9a23 into activeloopai:main Apr 7, 2022

farizrahman4u mentioned this pull request Apr 11, 2022

Added prettytable package code with licence #1589

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changed str return to include Tensor-Wise information for issue #1439. #1543

Changed str return to include Tensor-Wise information for issue #1439. #1543

neel2299 commented Mar 15, 2022 •

edited

CLAassistant commented Mar 15, 2022 •

edited

farizrahman4u left a comment

neel2299 commented Mar 15, 2022 •

edited

neel2299 commented Mar 15, 2022

farizrahman4u left a comment

neel2299 commented Mar 17, 2022

farizrahman4u left a comment

neel2299 commented Mar 27, 2022 •

edited

FayazRahman Mar 28, 2022

neel2299 Apr 3, 2022 •

edited

neel2299 commented Mar 28, 2022

farizrahman4u left a comment •

edited

farizrahman4u Apr 3, 2022

neel2299 commented Apr 5, 2022 •

edited

farizrahman4u commented Apr 5, 2022

neel2299 commented Apr 6, 2022

neel2299 commented Apr 6, 2022

tatevikh commented Apr 7, 2022

neel2299 commented Apr 8, 2022

Changed __str__ return to include Tensor-Wise information for issue #1439. #1543

Changed __str__ return to include Tensor-Wise information for issue #1439. #1543

Conversation

neel2299 commented Mar 15, 2022 • edited

🚀 🚀 Pull Request

Checklist:

Changes

CLAassistant commented Mar 15, 2022 • edited

farizrahman4u left a comment

Choose a reason for hiding this comment

neel2299 commented Mar 15, 2022 • edited

neel2299 commented Mar 15, 2022

farizrahman4u left a comment

Choose a reason for hiding this comment

neel2299 commented Mar 17, 2022

farizrahman4u left a comment

Choose a reason for hiding this comment

neel2299 commented Mar 27, 2022 • edited

FayazRahman Mar 28, 2022

Choose a reason for hiding this comment

neel2299 Apr 3, 2022 • edited

Choose a reason for hiding this comment

neel2299 commented Mar 28, 2022

farizrahman4u left a comment • edited

Choose a reason for hiding this comment

farizrahman4u Apr 3, 2022

Choose a reason for hiding this comment

neel2299 commented Apr 5, 2022 • edited

farizrahman4u commented Apr 5, 2022

neel2299 commented Apr 6, 2022

neel2299 commented Apr 6, 2022

tatevikh commented Apr 7, 2022

neel2299 commented Apr 8, 2022

Changed str return to include Tensor-Wise information for issue #1439. #1543

Changed str return to include Tensor-Wise information for issue #1439. #1543

neel2299 commented Mar 15, 2022 •

edited

CLAassistant commented Mar 15, 2022 •

edited

neel2299 commented Mar 15, 2022 •

edited

neel2299 commented Mar 27, 2022 •

edited

neel2299 Apr 3, 2022 •

edited

farizrahman4u left a comment •

edited

neel2299 commented Apr 5, 2022 •

edited