Weather units: nws to zamg (4/4)

[rianadon]

Proposed change

Uses the new weather conversion in the weather entity ( #59533) to convert weather values to the configured units.

Type of change

• Dependency upgrade
• Bugfix (non-breaking change which fixes an issue)
• New integration (thank you!)
• New feature (which adds functionality to an existing integration)
• Breaking change (fix/feature causing existing functionality to break)
• Code quality improvements to existing code or addition of tests

Additional information

• This PR fixes or closes issue: fixes #
• This PR is related to issue:
• Link to documentation pull request:

Checklist

• The code change is tested and works locally.
• Local tests pass. Your PR cannot be merged unless tests pass
• There is no commented out code in this PR.
• I have followed the development checklist
• The code has been formatted using Black (black --fast homeassistant tests)
• Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

• Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

• The manifest file has all fields filled out correctly.
  Updated and included derived files by running: python3 -m script.hassfest.
• New or updated dependencies have been added to requirements_all.txt.
  Updated by running python3 -m script.gen_requirements_all.
• For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
• Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

• No score or internal
• 🥈 Silver
• 🥇 Gold
• 🏆 Platinum

To help with the load of incoming pull requests:

• I have reviewed two other open pull requests in this repository.

[probot-home-assistant]

Hey there @fabaff, @freekode, @nzapponi, mind taking a look at this pull request as it has been labeled with an integration (openweathermap) you are listed as a code owner for? Thanks!
_{^{(message by CodeOwnersMention)}}

[probot-home-assistant]

Hey there @MatthewFlamm, mind taking a look at this pull request as it has been labeled with an integration (nws) you are listed as a code owner for? Thanks!
_{^{(message by CodeOwnersMention)}}

[MatthewFlamm]

As the tests indicate the data from NWS comes in different units for observations and forecasts currently. I believe there is a new option in the API to change the forecast units, but this isn't available in the upstream library yet. Until then, I don't think you can include NWS in this.

[rianadon]

As the tests indicate the data from NWS comes in different units for observations and forecasts currently. I believe there is a new option in the API to change the forecast units, but this isn't available in the upstream library yet. Until then, I don't think you can include NWS in this.

Apologies if I'm misunderstanding, but it looks like the data from pynws is converted to a set of fixed units. These will be configured as such in the weather entity:

    _attr_temperature_unit = TEMP_CELSIUS
    _attr_pressure_unit = PRESSURE_PA
    _attr_wind_speed_unit = SPEED_KILOMETERS_PER_HOUR
    _attr_visibility_unit = LENGTH_METERS

#59533 added unit conversion inside the base weather entity class, so these units will be converted back into the user's user system. It is a bit of extra work to have (for example - this applies to pressure/wind speed too) pynws convert Fahrenheit to Celsius then make the weather entitity base class convert Celsius to Fahrenheit; a cleaner solution would be to make temperature_unit return whatever unit the NWS api gives back. But it shouldn't break anything.

[MatthewFlamm]

Apologies if I'm misunderstanding, but it looks like the data from pynws is converted to a set of fixed units.

This is incorrect. The current observations are in one unit system. The forecasts are in a different set of units. Check the wind speed and temperature conversions in the existing nws weather module.

Temperature is covered to Fahrenheit here so that it matches the forecast unit.

core/homeassistant/components/nws/weather.py

Line 163 in 5907f66

return convert_temperature(temp_c, TEMP_CELSIUS, TEMP_FAHRENHEIT)

The temp is unchanged in the forecast.

core/homeassistant/components/nws/weather.py

Line 259 in 5907f66

ATTR_FORECAST_TEMP: forecast_entry.get("temperature"),

#59533 assumes that these units are the same.

[rianadon]

Thank you! I very misunderstood.

It looks like the forecast is in US units:

The component could do the conversion, but I'm thinking maybe it would be better to do this inside the pynws library so it returns the same set of units?

[MatthewFlamm]

Right forecast is currently in us units and observations in si units. Forecast can be configured to be in si units too (observations are not configurable), but there are some features like the textual detailed forecast which wouldn't be converted back to us units here.

It would be best for NWS API to support us and si units for both forecasts and observations. Second best would be for pynws to allow observations to be converted to us units. I'm not sure if it will get done, or should get done for this PR though.

[MatthewFlamm]

LGTM for NWS with some minor comments.

Based on the needed changes in the tests, it looks like the precision of the data is changing. This could be a breaking change if any users have an equality in their automations, although unlikely.

[rianadon]

I think breaking the automations is very unlikely. Using less than or greater than comparisons I can see, but not equality. Many of these PRs updating weather units are standardizing the precisions, so I think this would clutter release notes too much.

[frenck]

This is a massive breaking change.

Also:

• Opened up Revert "Weather units: climacell (1.3/4)" #63842 to revert Weather units: climacell (1.3/4) #61472, which does a similar thing as this PR.
• Opened up Revert "Weather units: Buienradar (1.2/4)" #63843 to revert Weather units: Buienradar (1.2/4) #61470, which does a similar thing as this PR.


[MatthewFlamm]

If this doesn't get merged or something similar than the entity model for weather continues to be broken. Should we additionally revert the earlier changes that convert units to non -supported values?

[frenck]

If this doesn't get merged or something similar than the entity model for weather continues to be broken

In all cases, it is broken. Adding these PRs is also massively breaking.

See also the revert PRs you have been tagged on: #63843

Unfortunately, we had no response.

[MatthewFlamm]

I have no strong feeling on how this is fixed. But #59533 introduced the first breaking change that changed the entity model (no breaking change was noted or architecture decision was made). At least these PRs bring back the units to match the entity model, but I understand there are other changes that are unwanted. If so should we revert #59533?

[frenck]

but I understand there are other changes that are unwanted.

You misunderstood, it is wanted. We just need to mitigate a breaking change for the end-user. While there is already a partly breaking change shipped, merging the change of all these units is a massive breaking change for all weather entities across the board.

[MatthewFlamm]

Can we modify #59533 to convert to the accepted units as part of the stated entity model? I'm trying to figure out how to address the broken units issue currently. The situation seems unclear to me.

[rianadon]

@frenck Sorry about the delays replying to this. I've had a lot going on personally and there's been a lot to keep up with.

As I understand, we had previously merged changes to the base weather entity that had made some more quantities like wind speed, pressure, etc determined by the global unit system.
In the process of making these changes, I think you wanted to reduce the number of quantities that depended on the global units system and instead suggested making units configurable per-entity or per-integration.
So we are reverting the integration PRs.

Is the path forwards to revert everything including the changes to the base weather entity, so we are back to where Home Assistant stood before these PRs? Or are you imagining a different way?