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

feat: use average estimated percentage on aggregated levels #7266

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

VIKTORVAV99
Copy link
Member

Issue

Right now we say that things are fully estimated even though they are not.

image

Description

If we take the estimation percentages for each data point and derive and average we instead get this:
image

Which more clearly communicate what we want it to.

Double check

  • I have tested my parser changes locally with poetry run test_parser "zone_key"
  • I have run pnpx prettier@2 --write . and poetry run format in the top level directory to format my changes.

@VIKTORVAV99
Copy link
Member Author

A question I have here is if we should change what is shown on a hourly view as well, we can easily calculate the estimated percentage and show that for those as well.

@Alportan
Copy link
Contributor

Alportan commented Oct 2, 2024

A question I have here is if we should change what is shown on a hourly view as well, we can easily calculate the estimated percentage and show that for those as well.

Do we know which sources are estimated on an hourly level? 😲 I had an impression that we can't determine that.

@Alportan
Copy link
Contributor

Alportan commented Oct 2, 2024

I'm in favour to simplify it and remove decimals as discussed in our Slack thread. Maybe we can discuss this briefly during our design review today! 🙌

@VIKTORVAV99
Copy link
Member Author

A question I have here is if we should change what is shown on a hourly view as well, we can easily calculate the estimated percentage and show that for those as well.

Do we know which sources are estimated on an hourly level? 😲 I had an impression that we can't determine that.

I yeah for the hourly view 😉

But we could change the way we do the other calculations in the backend as well to get the real estimated value on a hourly basis. It's just going to take some more effort than what we can do in this PR. But if we change it the app will use the new hourly values because how this is implemented.

@VIKTORVAV99
Copy link
Member Author

I'm in favour to simplify it and remove decimals as discussed in our Slack thread. Maybe we can discuss this briefly during our design review today! 🙌

I thought it just meant removing it below 1% but keeping it for above. I'm not sure I want to round it to 1% incements.

@Alportan
Copy link
Contributor

Alportan commented Oct 2, 2024

I'm in favour to simplify it and remove decimals as discussed in our Slack thread. Maybe we can discuss this briefly during our design review today! 🙌

I thought it just meant removing it below 1% but keeping it for above. I'm not sure I want to round it to 1% incements.

Ohh, maybe you're right 🙈 I might have misunderstood it!

@Alportan
Copy link
Contributor

Alportan commented Oct 2, 2024

A question I have here is if we should change what is shown on a hourly view as well, we can easily calculate the estimated percentage and show that for those as well.

Do we know which sources are estimated on an hourly level? 😲 I had an impression that we can't determine that.

I yeah for the hourly view 😉

But we could change the way we do the other calculations in the backend as well to get the real estimated value on a hourly basis. It's just going to take some more effort than what we can do in this PR. But if we change it the app will use the new hourly values because how this is implemented.

Let's maybe park this and discuss it for future improvements! 🙌

@VIKTORVAV99
Copy link
Member Author

@tonypls or @cadeban mind taking a look at this and adding your thoughts?

@Alportan
Copy link
Contributor

Alportan commented Oct 8, 2024

@VIKTORVAV99 let's aim for a first version which excludes showing any estimations below 1% and figure the more complex backend changes later on. 🙌

@VIKTORVAV99
Copy link
Member Author

@VIKTORVAV99 let's aim for a first version which excludes showing any estimations below 1% and figure the more complex backend changes later on. 🙌

I'm mostly interested if we should do it for hourly as well, we don't need any backend changes for that. We'd only need the backend changes to make the daily, monthly and yearly more accurate.

@Alportan
Copy link
Contributor

Alportan commented Oct 8, 2024

@VIKTORVAV99 let's aim for a first version which excludes showing any estimations below 1% and figure the more complex backend changes later on. 🙌

I'm mostly interested if we should do it for hourly as well, we don't need any backend changes for that. We'd only need the backend changes to make the daily, monthly and yearly more accurate.

Hmm, the thing is I never saw a partial estimate on an hourly level. Could you point me to a zone I can use as an example where the estimation is partial and under 1% for hourly? 🙏

@VIKTORVAV99
Copy link
Member Author

@VIKTORVAV99 let's aim for a first version which excludes showing any estimations below 1% and figure the more complex backend changes later on. 🙌

I'm mostly interested if we should do it for hourly as well, we don't need any backend changes for that. We'd only need the backend changes to make the daily, monthly and yearly more accurate.

Hmm, the thing is I never saw a partial estimate on an hourly level. Could you point me to a zone I can use as an example where the estimation is partial and under 1% for hourly? 🙏

For Greece only the last 2 hours are estimated, so that would be 8% of the whole graph. Right now it just says partially estimated.

@Alportan
Copy link
Contributor

Alportan commented Oct 8, 2024

@VIKTORVAV99 let's aim for a first version which excludes showing any estimations below 1% and figure the more complex backend changes later on. 🙌

I'm mostly interested if we should do it for hourly as well, we don't need any backend changes for that. We'd only need the backend changes to make the daily, monthly and yearly more accurate.

Hmm, the thing is I never saw a partial estimate on an hourly level. Could you point me to a zone I can use as an example where the estimation is partial and under 1% for hourly? 🙏

For Greece only the last 2 hours are estimated, so that would be 8% of the whole graph. Right now it just says partially estimated.

Ahh I see, so because the last 2 hours are estimated, we show the partially estimated label on the overview graph. In this case, I would only keep the card on the top and tooltips, and remove it completely from all overview graphs. I think this should solve it, and also keep things cleaner. ✨ Was this what you had in mind @VIKTORVAV99 ? 🙌

image

Once we make a decision regarding the Information architecture, we will come back to this and figure out the best way to indicate which data is preliminary, let's remove unnecessary things 🙌

@VIKTORVAV99
Copy link
Member Author

@VIKTORVAV99 let's aim for a first version which excludes showing any estimations below 1% and figure the more complex backend changes later on. 🙌

I'm mostly interested if we should do it for hourly as well, we don't need any backend changes for that. We'd only need the backend changes to make the daily, monthly and yearly more accurate.

Hmm, the thing is I never saw a partial estimate on an hourly level. Could you point me to a zone I can use as an example where the estimation is partial and under 1% for hourly? 🙏

For Greece only the last 2 hours are estimated, so that would be 8% of the whole graph. Right now it just says partially estimated.

Ahh I see, so because the last 2 hours are estimated, we show the partially estimated label on the overview graph. In this case, I would only keep the card on the top and tooltips, and remove it completely from all overview graphs. I think this should solve it, and also keep things cleaner. ✨ Was this what you had in mind @VIKTORVAV99 ? 🙌

image

Once we make a decision regarding the Information architecture, we will come back to this and figure out the best way to indicate which data is preliminary, let's remove unnecessary things 🙌

Either that, or we show the estimated percentage like we do on the other graphs. So instead of showing nothing or partially estimated we would show 8% estimated. This would keep it in line with the other aggregates.

@tonypls
Copy link
Collaborator

tonypls commented Oct 8, 2024

If we are applying a <1% rule then it could be nice to apply it in the same manner everywhere we show an estimated %, Germany yearly in this example.

image

@Alportan
Copy link
Contributor

Alportan commented Oct 9, 2024

@VIKTORVAV99 let's aim for a first version which excludes showing any estimations below 1% and figure the more complex backend changes later on. 🙌

I'm mostly interested if we should do it for hourly as well, we don't need any backend changes for that. We'd only need the backend changes to make the daily, monthly and yearly more accurate.

Hmm, the thing is I never saw a partial estimate on an hourly level. Could you point me to a zone I can use as an example where the estimation is partial and under 1% for hourly? 🙏

For Greece only the last 2 hours are estimated, so that would be 8% of the whole graph. Right now it just says partially estimated.

Ahh I see, so because the last 2 hours are estimated, we show the partially estimated label on the overview graph. In this case, I would only keep the card on the top and tooltips, and remove it completely from all overview graphs. I think this should solve it, and also keep things cleaner. ✨ Was this what you had in mind @VIKTORVAV99 ? 🙌
image
Once we make a decision regarding the Information architecture, we will come back to this and figure out the best way to indicate which data is preliminary, let's remove unnecessary things 🙌

Either that, or we show the estimated percentage like we do on the other graphs. So instead of showing nothing or partially estimated we would show 8% estimated. This would keep it in line with the other aggregates.

Your proposal actually sounds better when it comes to visible data errors, where a disclaimer that the graph contains preliminary/estimated data would be helpful. Hopefully we can show non-measured data directly on the graphs in the future and not have the label at all, but until then, we might want to keep it. 🙌

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.

3 participants