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

Relationship analysis chart in demo has no flexibility in Y axis #459

Open
1 task done
yury-fedotov opened this issue May 6, 2024 · 10 comments
Open
1 task done
Assignees
Labels
Community Issue/PR opened by the open-source community

Comments

@yury-fedotov
Copy link
Contributor

Description

If we look at relationship analysis page of the demo UI, it provides explicit controls of what variables to put on axis:

Screenshot 2024-05-06 at 9 58 43 PM

Those selectors are not created automatically on the plotly size - they are part of Vizro source code.

So on one hand there's flexibility in selecting variables to put on axis, but on the other hand, the range for Y axis is hard-coded.

This in fact makes the chart empty for all Y axis options except for life_expectancy, which is where those limits are coming from. What this leads to is that if a user makes the simplest adjustment, like flip X and Y axis, the chart becomes empty:

Screenshot 2024-05-06 at 10 01 52 PM

Because the range for Y axis is hard coded to be representative of life_expectancy values.

Expected behavior

The page should either not provide those controls, or make them functional so that the chart works with any combination of those without hard coded ranges for any.

Which package?

vizro

Package version

0.1.16

Python version

3.12

OS

MacOS

How to Reproduce

Added in the description.

Output

Added in the description.

Code of Conduct

@yury-fedotov yury-fedotov added Issue: Bug Report 🐛 Issue/PR that report/fix a bug Status: Needs triage 🔍 Issue/PR needs triaging labels May 6, 2024
@huong-li-nguyen
Copy link
Contributor

huong-li-nguyen commented May 7, 2024

Great point! 👍 🚀

I think it's best to remove these controls from that page. Are you keen to create a PR yourself? You would only have to update the app.py here and the jupyter version :)

Otherwise, let me know and I'll do it, but we always welcome contributions! 💯

@huong-li-nguyen huong-li-nguyen added Community Issue/PR opened by the open-source community and removed Issue: Bug Report 🐛 Issue/PR that report/fix a bug Status: Needs triage 🔍 Issue/PR needs triaging labels May 7, 2024
@huong-li-nguyen huong-li-nguyen self-assigned this May 7, 2024
@yury-fedotov
Copy link
Contributor Author

I'll be happy to try. Where can I find the contribution docs? I guess I'll need some developer setup, e.g. for this thing the team uses to generate the CHANGELOG from small files autogenerated for each PR.

I raised an issue that currently it leads to 404.

@huong-li-nguyen
Copy link
Contributor

huong-li-nguyen commented May 7, 2024

Here we go: https://vizro.readthedocs.io/en/stable/pages/explanation/contributing/

In general, it's just setting up hatch. The linting will be done automatically when you push the PR. The changelog you would create by running hatch run changelog:add from vizro-core :) Give it a try and let me know if there are any issues!

I've assigned the other ticket to our technical writer @stichbury as well :) She'll take care of the broken link 👍

@antonymilne
Copy link
Contributor

antonymilne commented May 7, 2024

FYI @yury-fedotov our contributions guidelines are a little bit out of date, and actually it should be even easier to install hatch now - see https://hatch.pypa.io/latest/install/. The new hatch python command also makes things even easier.

We should update these guidelines, not just because they're slightly out of date, but also because the vizro contribution process should be really easy (e.g. no need for DCO signoff like on kedro), and our docs are probably a bit overwhelming to a first time contributor at the moment and make it look harder than it actually is. @stichbury did you already have any plans for the contribution page? It's been on my list for a while to revisit.

@stichbury
Copy link
Contributor

@antonymilne There is indeed an issue in our list (738 on the internal queue, which I won't link to here) to revisit the contribution guide. I'll need some engineering time to help me with it, so maybe next sprint we can block out a few hours?

@yury-fedotov
Copy link
Contributor Author

Thanks for the replies guys :) Quick question on PRs that fix minor things, like this one.

For such small updates / bugfixes you don't do the CHANGELOG thing with skriv?

@antonymilne
Copy link
Contributor

antonymilne commented May 8, 2024

Actually all PRs should currently require a changelog file, even if it's left empty. It looks like the checks-vizro-core job that enforces this didn't run on that PR - not sure why. Probably it just got removed by accident and didn't get added to the list again. Any ideas @huong-li-nguyen @l0uden?

With that said, PRs that are just changes to docs don't really need a changelog file at all, and it would be nice to not force an author to generate one. We could probably change the job to not apply the job for PRs with the docs/ prefix 🤔 Or check to see if the only files changed were in docs. wdyt @maxschulz-COL? Could be a quick and easy improvement for this flow.

@huong-li-nguyen
Copy link
Contributor

huong-li-nguyen commented May 8, 2024

No, it actually doesn’t require a changelog file because it doesn’t contain changes inside the folders vizro-core or vizro-ai :)

Otherwise the linting would complain about a missing changelog file. So you can do any changes outside these folders without a changelog file being required.

I didn’t remove the test. If it didn’t run, it’s probably because it didn’t get triggered as there are no changes inside the relevant folders.

@antonymilne
Copy link
Contributor

antonymilne commented May 8, 2024

Ahah yes, you are right! I thought the job always run but you're right that it doesn't get triggered unless there's files changed inside vizro-core or vizro-ai 👍 Nothing broken then. So basically @yury-fedotov, that PR is just an edge case where the check that a changelog file was added didn't run - in 99% of PRs, even simple ones, a check will block you from merging without a changelog file (an empty file is ok).

@antonymilne
Copy link
Contributor

FYI after has #480 merged then a changelog fragment should no longer be required just for docs edits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community Issue/PR opened by the open-source community
Projects
None yet
Development

No branches or pull requests

4 participants