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

Include JSON dataset in the demo-project #1930

Merged
merged 10 commits into from
May 30, 2024

Conversation

SajidAlamQB
Copy link
Contributor

@SajidAlamQB SajidAlamQB commented May 29, 2024

Description

Related to: #1929

Development notes

Replaced companies.csv with companies_json.json and added a new node that converts the json into a csv. This should make the json pre-viewable in the demo.

This is what it looks like with the new companies_json.json in the view

Edit:

After reviewing with Rashida we now have a new reporting node get_top_shuttles_data that takes the head of the input table and converts it into JSON. See below for results:

image

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
@SajidAlamQB SajidAlamQB self-assigned this May 29, 2024
@SajidAlamQB SajidAlamQB requested a review from Huongg May 29, 2024 11:44
SajidAlamQB and others added 3 commits May 29, 2024 13:09
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
@SajidAlamQB SajidAlamQB marked this pull request as ready for review May 29, 2024 12:45
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
@ravi-kumar-pilla
Copy link
Contributor

Hi @SajidAlamQB ,

I see that #1929 suggests to replace companies to json. I think it would be nice to introduce a new dataset rather than modifying existing datasets in the demo_project.

Thank you

@SajidAlamQB
Copy link
Contributor Author

I see that #1929 suggests to replace companies to json. I think it would be nice to introduce a new dataset rather than modifying existing datasets in the demo_project.

I'm not opposed but I feel like adding another dataset would be a bit random. If we wanted to include it for this demo we would have to rework it to make it feel natural.

Signed-off-by: huongg <huongg1409@gmail.com>
Signed-off-by: huongg <huongg1409@gmail.com>
Copy link
Contributor

@ravi-kumar-pilla ravi-kumar-pilla 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 the PR @SajidAlamQB . Left a minor comment, otherwise looks fine.

Copy link
Contributor

@Huongg Huongg left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @SajidAlamQB

Copy link
Contributor

@rashidakanchwala rashidakanchwala left a comment

Choose a reason for hiding this comment

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

Hi team,

Not sure if this is the best example of previewing JSONDatasets on Kedro-viz. It might be beneficial to provide more illustrative examples so that users can better understand how to use it. The current JSON is a direct conversion of a really long table. Ideally, we could slice it to show the top 5 or 6 rows and represent that in a JSON format.

Thanks!

Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
@SajidAlamQB
Copy link
Contributor Author

After reviewing with Rashida we now have a new reporting node get_top_shuttles_data that takes the head of the input table and converts it into JSON. See below for results:

image

@SajidAlamQB SajidAlamQB enabled auto-merge (squash) May 30, 2024 10:37
@SajidAlamQB SajidAlamQB disabled auto-merge May 30, 2024 10:37
@rashidakanchwala rashidakanchwala self-requested a review May 30, 2024 10:42
@SajidAlamQB SajidAlamQB merged commit 979d2e6 into main May 30, 2024
6 checks passed
@SajidAlamQB SajidAlamQB deleted the feat/update-demo-to-include-json branch May 30, 2024 10:44
@Huongg Huongg mentioned this pull request May 30, 2024
5 tasks
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.

6 participants