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: convert EUR/MWh to EUR/KWh #7023

Merged
merged 4 commits into from
Aug 13, 2024
Merged

Conversation

VIKTORVAV99
Copy link
Member

@VIKTORVAV99 VIKTORVAV99 commented Jul 30, 2024

Issue

Closes: AVO-443

Description

Creates a helper function to convert prices (which currently only handles EUR) and uses that to convert EUR/MWh to EUR/KWh

  • Add tests for the new function to ensure it's working as expected.

Double check

  • I have run pnpx prettier@2 --write . and poetry run format in the top level directory to format my changes.

): { value?: number; currency?: string; unit: EnergyUnits } => {
if (currency === 'EUR') {
if (value) {
value = round(value / 1000, 4);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be 2dp as a cent is the most granular unit of the currencies we use

Copy link
Member Author

Choose a reason for hiding this comment

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

I did some research and testing and the problem with 2 decimal points is that we loose a lot of information.

For example if the price is 0,56 EUR per MWh it would be 0,0006 EUR per kWh if using 4 decimals but 0.01 EUR per kWh if we used 2 decimals.

The rounding to 4 decimals causes an error of 0.6 if converted back to per MWh but if the same is done with 2 decimals the error would be 9.44, which is very bad.

Other sites that show the price per kWh shows it with 4 decimals as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Alportan with the above in mind you to think it makes sense to have a toggle in the settings to show prices as per MWh or per kWh?

I would assume some energy experts and the like want to have the most accurate value all the time, even if it's per MWh?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, good point! I think a toggle in the settings does make a lot of sense. Should I sketch something? 🙌

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, good point! I think a toggle in the settings does make a lot of sense. Should I sketch something? 🙌

If you wish!

I'm thinking that maybe we should try and move stuff into a settings modal next cycle. Even if it's just the current stuff and then we can expand from there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Happy for this to go through as it is if you're happy with the 4dp

Copy link
Collaborator

@tonypls tonypls left a comment

Choose a reason for hiding this comment

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

LTGM, will leave it up to you if you want to do further changes here then tag me other wise a V2 branch works 👍

@VIKTORVAV99
Copy link
Member Author

I think I'll merge this as is and then I'll sync a bit with Alex on the settings modal and how we can best incorporate a setting with that.

@VIKTORVAV99 VIKTORVAV99 enabled auto-merge (squash) August 13, 2024 16:16
@VIKTORVAV99 VIKTORVAV99 merged commit 9ec2945 into master Aug 13, 2024
21 checks passed
@VIKTORVAV99 VIKTORVAV99 deleted the vik/convert_eur/mwh_to_eur/kwh branch August 13, 2024 16:20
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