mckinsey / vizro

Vizro is a toolkit for creating modular data visualization applications.
https://vizro.readthedocs.io/en/stable/
Apache License 2.0
2.63k stars 118 forks source link

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

Closed yury-fedotov closed 3 months ago

yury-fedotov commented 4 months ago

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

huong-li-nguyen commented 4 months ago

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! 💯

yury-fedotov commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

FYI after has https://github.com/mckinsey/vizro/pull/480 merged then a changelog fragment should no longer be required just for docs edits.