Noaaweather#23875
Conversation
|
Hi @rewsssiestedbo, It seems you haven't yet signed a CLA. Please do so here. Once you do that we will be able to review and accept this pull request. Thanks! |
|
This needs to be considered along with the weather platform PR in #23647. I think there should be consistent naming and methodology. I propose NWS since it the sub-agency of NOAA that provides the data. I have no strong position here however. There are also several different methods that were chosen in our approaches. For example, here, the HA condition is derived through some rules from the long text description while the weather platform PR uses the icon url provided in the nws data. These will sometimes give inconsistent results. I like the icon url approach as it is straightforward, but I'd like to hear if there is a benefit to your approach that I haven't considered. Another example is that you use the raw metar code as the fallback for missing or low quality data. The weather platform PR does not use a fall back at all currently and in fact will use data marked as poor quality. I planned on falling back to the previous observation in this case, to be introduced in a future PR. We should align on a common fallback methodology. I propose that we work together to iron out these differences, starting with the above two points, but I will read your PR in more detail. |
Yes, we should work together on this. I have no particular concern about using nws instead of noaa, If I recall, the only reason I went with noaa it seemed less likely to have a conflict with some other countries weather service abbreviation, and there was already the NOAA Tides sensor. Finally, about dealing with missing information in responses. I initially just had the code to avoid updating a condition if the response was missing a value for that condition. However, for a few stations I was monitoring (PHNG and PHNL show this), there would be long periods with no valid temperature value (for example) in the temperature field, but with an updated value in the METAR record. It is not clear to me why this happens, but it is not unusual for there to be multiple observation records without valid values for a number of conditions, but with valid values in the METAR record. I'll try to take a look are your PR and see how best to align. |
|
I've noticed too that sometimes some stations are better than others. The closest station to me is always sunny or cloudy, but never rainy. My nearby stations will only be missing/contain poor quality data for the current observation, however. I think your metar fall back is the right approach for your case, but adds quite the complexity to the HA code base. It seems to me like it would be better to have these decisions unified in one module(or a new pypi library) rather than spread across two platforms. I need to think about the path forward, but one suggestion is to co-create a new pypi library that does all the data cleansing and presents HA with a very clean API. For the icon, the big upside I see is that they publish a list of icons. So if they change the icon names and they continue to publish this list, it will be easy to maintain. Your method could be a nightmare if it ever changes. You'd have to deduce the new structure of the text. |
Note that the translation to the HA weather code values was the last thing I added, and is only used as an option, since the NWS text descriptions have much more content than the HA codes, and I would rather have the direct NWS text. Any thoughts on how to get started on creating the separate (from HA) module(s)? For the forecast data, had you looked at using the NDFD API instead of the one you are using? It would be somewhat more complex to handle the XML responses, but it provides much more data, including precipitation amounts, dewpoint, cloud cover, etc. |
|
After reading through your PR again, it seems like the inconsistencies are mainly the ones I already mentioned, so I don't think we need a separate library for this. The biggest decision is to coalesce on the name in my mind. I will look into implementing a metar fallback in my PR. Do you have a reference for the qc code info you used?
I did look at it, but it again adds huge complexity. The weather.gov API is very simple and clean. |
I was never able to find any documentation on the qc codes. The comments in sensors.py are a result of reverse engineering/review of the results from a number of different stations over a few months. For the HA conditions, it appears that the differences in the results from using the icons versus the text are for:
As far as the name, it makes sense to use nws.
|
| """Convert the measurement from the native unit to the appropriate unit. | ||
|
|
||
| This will do the appropriate conversion to get the value | ||
| from unit it is supplied in to the standard metric or imperial |
There was a problem hiding this comment.
This whole function should use the built-in unit conversions, see pressure comment above.
There was a problem hiding this comment.
Changed to use the built in unit conversions for temperature and pressure. As the distance one does not have inches for millimeters, and there are no speed conversions, I left those as is. In a future pull request I may try to add the builtin conversion for speed and the additional distance units.
this should have been handled in the update code already. Add HA weather condition of "hail". While this is not in the documentation it is in the demo component.
…hs of the code that depend upon observation data: either the time of day or the values available.
Use specific data for night time check. Note this also tests duplicate observation timestamps.
… instead of embedded error
Update timestamps in test data to ensure duplicate timestamps.
…f code handling those items missing
…n when bad parameters are given
|
I added the metar fallback logic into the upstream I'm willing to consider more complex logic in the pynws library, too if warranted. |
|
Closing to wait for integration of nws weather component, and then integration of the sensor code with that component. |
Description:
Add sensor to get weather measurements from US NOAA/National Weather Service
Related issue (if applicable): fixes #
Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>
Not yet submitted
Example entry for
configuration.yaml(if applicable):Checklist:
tox. Your PR cannot be merged unless tests passIf user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
python3 -m script.hassfest.requirements_all.txtby runningpython3 -m script.gen_requirements_all..coveragerc.If the code does not interact with devices: