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

Increase significant digits #6173

Merged

Conversation

silimotion
Copy link
Contributor

Issue

Closes #5921

Description

This PR increases the default number of digits used by the tooltips from 2 to 3. I believe this is better as it shows more detailed data, but it's open to discussion.

Preview

Before:
Screenshot_20231124_115544

After:
Screenshot_20231124_012718

Double check

  • I have run pnpx prettier --write . and poetry run format to format my changes.

VIKTORVAV99
VIKTORVAV99 previously approved these changes Nov 25, 2023
Copy link
Member

@VIKTORVAV99 VIKTORVAV99 left a comment

Choose a reason for hiding this comment

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

This makes sense to me but I think it would be good to have a second pair of eyes take look at this as well.

So @madsnedergaard, @tonypls and @Alportan what do you guys think?

Copy link
Member

@madsnedergaard madsnedergaard left a comment

Choose a reason for hiding this comment

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

I overall think it makes sense, except for a single use case - the x-axis label on the breakdown chart:

Screenshot 2023-11-27 at 10 51 43

As an alternative, we can make the number optionally adjustable and then just change it for that specific place? :)

@Alportan
Copy link
Contributor

It does make sense to increase the accuracy of our data by adding decimals. 🙌

💡 Idea for the future: Agree on the number of decimals we display for different data points to ensure visual consistency.

@VIKTORVAV99 VIKTORVAV99 dismissed their stale review November 27, 2023 19:05

Changes needed

Copy link
Member

@VIKTORVAV99 VIKTORVAV99 left a comment

Choose a reason for hiding this comment

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

Looks like we all agree this would be a nice change.

However if you could update the chart to use 2 digits only that would be great!

@silimotion
Copy link
Contributor Author

I have set the BreakdownChart to 2 digits now because I've seen some zones in the map with labels such as "15 GW" that could be incorrectly rounded. Are there any checks in the code so that the calculated values in the labels only need 2 digits and never more?

@VIKTORVAV99
Copy link
Member

I have set the BreakdownChart to 2 digits now because I've seen some zones in the map with labels such as "15 GW" that could be incorrectly rounded. Are there any checks in the code so that the calculated values in the labels only need 2 digits and never more?

I do not think there are any calculated checks this in particular but since we are already using 2 decimals this should not cause any immediate issues.

Copy link
Member

@VIKTORVAV99 VIKTORVAV99 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 for making the last changes!

@VIKTORVAV99
Copy link
Member

@madsnedergaard does this adress your feedback? Turns out we had already made it possible to set the significants digits in specific places 🙂

@madsnedergaard
Copy link
Member

@madsnedergaard does this adress your feedback? Turns out we had already made it possible to set the significants digits in specific places 🙂

Yes, let's merge it :)

@madsnedergaard madsnedergaard enabled auto-merge (squash) December 4, 2023 11:30
@madsnedergaard madsnedergaard merged commit 43e3685 into electricitymaps:master Dec 4, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reduced data precision - significant digits on generation by source
4 participants