Skip to content

Add pressure mmhg unit conversion #35575

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.

Sign up for GitHub

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

Closed
Limych wants to merge 2 commits into from
Closed

Add pressure mmhg unit conversion #35575

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
576cbb1
Add mmHg pressure units (usual units for CIS)
Limych May 12, 2020
9b60ef1
Add tests for mmHg pressure unit
Limych May 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions homeassistant/const.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,6 +397,7 @@
PRESSURE_MBAR: str = "mbar"
PRESSURE_INHG: str = "inHg"
PRESSURE_PSI: str = "psi"
PRESSURE_MMHG: str = "mmHg"

# Volume units
VOLUME_LITERS: str = "L"
Expand Down
4 changes: 3 additions & 1 deletion homeassistant/util/pressure.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,20 @@
PRESSURE_PA,
PRESSURE_PSI,
UNIT_NOT_RECOGNIZED_TEMPLATE,
PRESSURE_MMHG,
)

_LOGGER = logging.getLogger(__name__)

VALID_UNITS = [PRESSURE_PA, PRESSURE_HPA, PRESSURE_MBAR, PRESSURE_INHG, PRESSURE_PSI]
VALID_UNITS = [PRESSURE_PA, PRESSURE_HPA, PRESSURE_MBAR, PRESSURE_INHG, PRESSURE_PSI, PRESSURE_MMHG]
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare May 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Is anything using this code, ie the new unit?

Copy link
Copy Markdown
Contributor Author

@Limych Limych May 13, 2020

Choose a reason for hiding this comment

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

No, there is no code yet that uses this.
But in the forum I met a lot of requests to add these units. And I myself am more familiar with them. That's why I added them.

Copy link
Copy Markdown
Contributor Author

@Limych Limych May 13, 2020

Choose a reason for hiding this comment

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

mmHg is usual pressure unit for CIS sitizens

Copy link
Copy Markdown
Member

@frenck frenck May 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

So if nothing is using it, why should it be added? As it sounds like this will add "dead" code to our codebase.

Copy link
Copy Markdown
Contributor Author

@Limych Limych May 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The reason is that this (and only this) unit of pressure measurement is familiar to the CIS population (and this is more than 280 million people). More precisely -- all former regions of the USSR.
Yes, this unit is not currently used in the Home Assistant code. But in the forum, people regularly ask to add it, for example, to the weather frontend.

What is the point of making “crutches” in separate modules when you can centrally add this unit to the core and solve the problem?

Copy link
Copy Markdown
Contributor Author

@Limych Limych May 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Just because you live in a country where other standards are generally accepted, gives no reason to ignore the standards generally accepted in countries on the other sixth of all the Earth’s land.

Yes, that integration developer could theoretically suggest this code. But for some reason he did not. Maybe because he didn’t even think that it was possible? Or maybe because the conversion of pressure units was added to the core code later than he did his component?

Adding this unit will allow this and other developers to make their work easier in the future. It will allow in the future users of Home Assistant from the CIS countries to use this system with great comfort.

I am sure that a little time will pass and this code will be claimed by developers and will cease to be dead.

Copy link
Copy Markdown
Contributor Author

@Limych Limych May 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

By the way, I also found an example when a person needs reverse conversion: https://community.home-assistant.io/t/change-the-unit-in-bloomsky-sensors/35837

Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare May 13, 2020

Choose a reason for hiding this comment

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

We won't accept this PR until there's an active use case in the code.

Copy link
Copy Markdown
Contributor

@asmfreak asmfreak May 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Here is a use-case out-of-code. I, as many others, want to customize output of any weather sensor, which I'm not using ATM partially because there is no pressure unit, used in my country. I could use a template, but it would be a lot nicer to put one small setting in one sensor to make it work like all other system.
Please, put yourself in our shoes and image if your country's proffered pressure/temperature/whatever unit was not supported. Let's imagine for a second, that HA was originally developed not in the US, but somewhere in Europe with metric system. I really don't think that a US citizen coming with such a small proposal adding their unit of temperature measurement, would be rejected so harshly. There was a time, when there were no temperature units in HA, and it was corrected.
As for code usage - there are CIS weather services, such as Gismeteo and Yandex Weather, providing pressure in mmHg, not in hPa. And integrations for them could have used mmHg for their operation. Please see this integration's example code. This code could have used the proposed unit.

Copy link
Copy Markdown
Member

@frenck frenck May 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is about adding "dead" code to our codebase. As soon as there is a use case for it in our codebase, we are happy to accept and add it.

The integration you have linked it not the Home Assistant codebase. Feel free to open up a PR to add an integration to our codebase that add this and the new integration.

PS: Did you know most of us are not in the US? Like everybody who reviewed this PR? Thanks.


UNIT_CONVERSION = {
PRESSURE_PA: 1,
PRESSURE_HPA: 1 / 100,
PRESSURE_MBAR: 1 / 100,
PRESSURE_INHG: 1 / 3386.389,
PRESSURE_PSI: 1 / 6894.757,
PRESSURE_MMHG: 1 / 133.3224,
}


Expand Down
22 changes: 22 additions & 0 deletions tests/util/test_pressure.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
PRESSURE_MBAR,
PRESSURE_PA,
PRESSURE_PSI,
PRESSURE_MMHG,
)
import homeassistant.util.pressure as pressure_util

Expand All @@ -20,6 +21,7 @@ def test_convert_same_unit():
assert pressure_util.convert(3, PRESSURE_HPA, PRESSURE_HPA) == 3
assert pressure_util.convert(4, PRESSURE_MBAR, PRESSURE_MBAR) == 4
assert pressure_util.convert(5, PRESSURE_INHG, PRESSURE_INHG) == 5
assert pressure_util.convert(6, PRESSURE_MMHG, PRESSURE_MMHG) == 6


def test_convert_invalid_unit():
Expand Down Expand Up @@ -69,3 +71,23 @@ def test_convert_from_inhg():
assert pressure_util.convert(inhg, PRESSURE_INHG, PRESSURE_MBAR) == pytest.approx(
1015.9167
)


def test_convert_from_mmhg():
"""Test conversion from mmHg to other units."""
mmhg = 745
assert pressure_util.convert(mmhg, PRESSURE_MMHG, PRESSURE_PSI) == pytest.approx(
14.40585734588
)
assert pressure_util.convert(mmhg, PRESSURE_MMHG, PRESSURE_HPA) == pytest.approx(
993.25188
)
assert pressure_util.convert(mmhg, PRESSURE_MMHG, PRESSURE_PA) == pytest.approx(
99325.188
)
assert pressure_util.convert(mmhg, PRESSURE_MMHG, PRESSURE_MBAR) == pytest.approx(
993.25188
)
assert pressure_util.convert(mmhg, PRESSURE_MMHG, PRESSURE_INHG) == pytest.approx(
29.33069826777
)