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

[Demo] Add diverging bar to visual-vocabulary #792

Merged
merged 12 commits into from
Oct 11, 2024

Conversation

hxe00570
Copy link
Contributor

@hxe00570 hxe00570 commented Oct 5, 2024

Description

  • Add diverging bar to visual-vocabulary

Screenshot

Screenshot 2024-10-10 at 15 01 29

Notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

    • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
    • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorized to submit this contribution on behalf of the original creator(s) or their licensees.
    • I certify that the use of this contribution as authorized by the Apache 2.0 license does not violate the intellectual property rights of anyone else.
    • I have not referenced individuals, products or companies in any commits, directly or indirectly.
    • I have not added data or restricted code in any commits, directly or indirectly.

@nadijagraca
Copy link
Contributor

nadijagraca commented Oct 7, 2024

Hi @hxe00570, thank you for contributing. 🙌

There are several things that need to be added to this PR for it to be merged.
I've summarised these things in the to-do list below:

  • create complete custom_chart example for diverging bar chart and add it to a page titled "Diverging bar".
  • update README.md file
  • Add changelog file
  • Add chart to relevant category, also remove the chart from incomplete charts.
  • update PR description with chart description and screenshot

Also, you could try check and fix the 3 failed CI?

  • Checks for Vizro / checks-vizro-core (pull_request)
  • Lint / lint-vizro-all (pull_request)
  • pre-commit.ci - pr

Can you please advise if you wish to continue working on this issue, or some other contributor can take it from here?

Thank you, 🙏

@hxe00570
Copy link
Contributor Author

hxe00570 commented Oct 7, 2024

Thank you, I think I corrected this. Please inform

@nadijagraca
Copy link
Contributor

Thank you, I think I corrected this. Please inform

Can you please push your latest changes as I cannot see new commits.

Thank you,

@hxe00570
Copy link
Contributor Author

hxe00570 commented Oct 8, 2024

I have tried a few times to push and I keep getting different errors (this is my first contribution so I’m sure it’s user error). I have been using the Codespace. Any chance yall are on site at GHC and someone might be willing to meet up and help me?

@lingyielia
Copy link
Contributor

Hi @hxe00570 , we are not participating GHC in person this time. I have DMed you via Linkedin in case you'd like to chat. 😄

In the meantime, I saw another PR from you also related to diverging bar (#795). If they are duplicate, we can close one and keep working on one git branch.

@huong-li-nguyen
Copy link
Contributor

I have tried a few times to push and I keep getting different errors (this is my first contribution so I’m sure it’s user error). I have been using the Codespace. Any chance yall are on site at GHC and someone might be willing to meet up and help me?

Hello @hxe00570 ,

I just looked at the chart you created in this PR: #795 and it looks beautiful! ⭐ I also noticed that you tackled the more challenging task of creating a diverging stacked bar chart! 🚀

If you find yourself short on time or encountering issues with the development setup, we're happy to help you wrap this up within your branch. This way, you'll still receive credit for the contribution, and we can handle any remaining technical details. Please let me know if you'd prefer to complete this on your own or if you'd like us to assist in finishing it up within your branch :)

@hxe00570
Copy link
Contributor Author

hxe00570 commented Oct 9, 2024

@huong-li-nguyen thank you! Yes, that is totally fine with me! I'd like to volunteer to attempt some others too if that is available. Thanks! H

@lingyielia
Copy link
Contributor

@nadijagraca @huong-li-nguyen Had a quick chat with @hxe00570 today and she is ready to handover the pr to Vizro team now.

I will close this PR (#792) and we can assist in finishing it up in PR #795. 🙌

@huong-li-nguyen
Copy link
Contributor

Reopening this PR because @hxe00570 actually worked on the diverging stacked bar chart, so I'll use the two PRs to finish off 2 charts: the diverging bar chart and diverging stacked bar chart :)

@huong-li-nguyen huong-li-nguyen changed the title initial commit for diverging bar [Demo] Add diverging bar chart to visual-vocabulary Oct 10, 2024
@huong-li-nguyen huong-li-nguyen linked an issue Oct 10, 2024 that may be closed by this pull request
@huong-li-nguyen
Copy link
Contributor

@hxe00570 - I really liked your pastry example, so I've kept the data but I just replaced the measure with the profit ratio (fits better with negative and positive values) :)

You tackled the more complex chart which is the diverging stacked bar chart - so don't worry I'll re-use the code you created in another PR: #795

For the diverging bar chart, we were actually just looking for a simple sorted bar chart. So, in this PR, I am simplifying the function call by using the pure px.bar call 👍

@huong-li-nguyen huong-li-nguyen changed the title [Demo] Add diverging bar chart to visual-vocabulary [Demo] Add diverging bar to visual-vocabulary Oct 10, 2024
Copy link
Contributor

@antonymilne antonymilne left a comment

Choose a reason for hiding this comment

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

Thank you for all your work on this @hxe00570! 🙏

@huong-li-nguyen I think there's something small we should change here - see comment. But otherwise looks good! :shipit:

vizro-core/examples/visual-vocabulary/pages/deviation.py Outdated Show resolved Hide resolved
Copy link
Contributor

@nadijagraca nadijagraca left a comment

Choose a reason for hiding this comment

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

The chart looks great, well done @hxe00570 and @huong-li-nguyen . 🚀

@huong-li-nguyen huong-li-nguyen merged commit 3ca9193 into mckinsey:main Oct 11, 2024
30 checks passed
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.

Contribute Diverging bar to Vizro visual vocabulary
5 participants