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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added prettytable package code with licence #1589

Closed

Conversation

SaiNikhileshReddy
Copy link

@SaiNikhileshReddy SaiNikhileshReddy commented Apr 11, 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

I have added Pretty Table package to the hub by copying and adding licence to hub files. This package is being added directly into hub is to reduce package count and help this issue effectively.

@CLAassistant
Copy link

CLAassistant commented Apr 11, 2022

CLA assistant check
All committers have signed the CLA.

@SaiNikhileshReddy
Copy link
Author

@davidbuniat I have added PrettyTable code to the hub. If this is merged, I can push the code to the pretty print issue.

Tagging @farizrahman4u @mikayelh

@SaiNikhileshReddy
Copy link
Author

This PR adds backend code to print data in pretty tables. Below is an example of generated table using this PR code.
image

@farizrahman4u
Copy link
Contributor

Sorry but this issue has already been addressed by #1543 .

@SaiNikhileshReddy
Copy link
Author

@farizrahman4u This PR aims to solve the issues @davidbuniat has mentioned here.
My solution works without the usage of .summary() and also working on the function .schema() as described in the above issue.

@farizrahman4u
Copy link
Contributor

farizrahman4u commented Apr 11, 2022

@SaiNikhileshReddy I would still prefer @neel2299 's implementation as it does not add any additional dependencies nor does it copy the entire codebase of a library to hub repo (which is really bad for a simple feature like this). API wise, ds.summary() is more natural (similar to model.summary() in keras, but thats just api and can be done using your implementation too). If you want to make improvements to @neel2299 's implementation (which is already in main) without introducing additional dependencies that would be great!

@SaiNikhileshReddy
Copy link
Author

SaiNikhileshReddy commented Apr 12, 2022

@farizrahman4u Thanks for sharing the feedback and @neel2299 implementation is good.

I was aware of dependencies issues with external packages, but I was thinking in different perspective of having pandas like table generation which supports jupyter dataframe and even cli appearance without uisng external package within hub.

I contacted @davidbuniat about this and he asked me to integrate the code from the PrettyTable package as there were no dependencies to their implementation and the licence supports code modification.

It is my bad that I tried to add entire code instead of stripping unnecessary code for using internally in hub. I didn't want to reinvent the wheel again to generate table.

The implementation I was trying to do was having flexibility in generating tables for both overall dataset and for each tensor without calling ds.summary() by calling just ds and ds[tensor][1:10] like @davidbuniat mentioned in the examples of the issue.

I'll probably work on ds.schema() if the current implementation of ds.summary() is sufficiant.

Once again thanks for helping out. @farizrahman4u @mikayelh @davidbuniat

@farizrahman4u
Copy link
Contributor

@SaiNikhileshReddy I do not understand the "without calling ds.summary() by calling just ds".. doesn't that involve just adding it to __str__ of dataset and tensor?

@SaiNikhileshReddy
Copy link
Author

Yes, we can integrate __str__ directly into the ds. I was mentioning about using table code I suggested for both ds and for ds[tensor] which can even be used for any other purpose later.

@SaiNikhileshReddy
Copy link
Author

SaiNikhileshReddy commented Apr 16, 2022

I wanted to make it simpler for anyone working with tables in the future with standard API internally in the hub.

My main motivation for doing this was that @davidbuniat had mentioned in the issue to learn how pandas are implemented and try to source code for the hub. Instead of reinventing the table generation for quick completion of the problem.

@SaiNikhileshReddy
Copy link
Author

As now the problem has been solved, I'll try to push the code for ds.schema() like I mentioned in the issue.

Thanks for valuable feedback : )

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.

None yet

3 participants