Skip to content

1st class support for relative humidity in forecasts#117

Merged
MatthewFlamm merged 5 commits into
MatthewFlamm:masterfrom
lymanepp:ha-humidity
Jul 2, 2023
Merged

1st class support for relative humidity in forecasts#117
MatthewFlamm merged 5 commits into
MatthewFlamm:masterfrom
lymanepp:ha-humidity

Conversation

@lymanepp
Copy link
Copy Markdown
Collaborator

@lymanepp lymanepp commented Jun 29, 2023

Add first class support for relative humidity in NWS forecasts. This PR is a WIP right now and not ready to merge.

Remaining tasks are:

  • Recapture test data
  • Add new tests
  • Add other fields of interest (e.g., wind gust, and apparent temp)

@MatthewFlamm
Copy link
Copy Markdown
Owner

Going to make this draft, so that I know when to take a closer look.

Add other fields of interest (e.g., wind gust, and apparent temp)

My comment in homeassistant was for the code there. I think these (wind gust, wind chill, and heat index) are already in the observations. If possible, we can add all the newly added forecast types here.

@MatthewFlamm MatthewFlamm marked this pull request as draft June 30, 2023 00:11
@lymanepp
Copy link
Copy Markdown
Collaborator Author

lymanepp commented Jun 30, 2023

@MatthewFlamm, couple observations/questions for you.

  • The NWS responses have changed quite a bit since the JSON responses were last captured. The only thing I've found that might affect pynws are the units of measurement changes.
  • Do you have a script that is used to capture and/or manipulate the 22 stored JSON responses for the test cases?
  • There are a number of test cases that have to be changed whenever the stored JSON responses are updated. Ideally, assertions would be flexible enough that very few would need to be updated when test data is updated.

I can assist with adding new values and adjusting to API changes, but I'm not very interested in updating the test case JSON responses or adjusting the assertions.

@MatthewFlamm
Copy link
Copy Markdown
Owner

Do you have a script that is used to capture and/or manipulate the 22 stored JSON responses for the test cases?

No. The responses were made by grabbing them and then modifying them for specific purposes. A generation script sounds great in hindsight, but we do not have that now.

There are a number of test cases that have to be changed whenever the stored JSON responses are updated. Ideally, assertions would be flexible enough that very few would need to be updated when test data is updated.

I think we just need to add the new responses to a single json file for each type. So for forecasts, we would have to manually append those values in gridpoints_forecast.json and the hourly one. If you are not inclined to add those, I can push changes to this PR. Most of the JSON files are observations and I don't think you will touch those here.

@lymanepp lymanepp marked this pull request as ready for review July 2, 2023 01:13
@lymanepp
Copy link
Copy Markdown
Collaborator Author

lymanepp commented Jul 2, 2023

I noticed that you tweaked the test data, so I added "probabilityOfPrecipitation", "dewpoint", and "relativeHumidity" to the forecasts.

I didn't copy your test data changes to my fork, so you'll need to merge this into your branch.

@lymanepp lymanepp changed the title 1st class support for relative humidity in forecasts (WIP) 1st class support for relative humidity in forecasts Jul 2, 2023
@MatthewFlamm
Copy link
Copy Markdown
Owner

This is looking good, Thanks!

I need to think about this a little bit, but I noticed that the units for dewpoint are always in SI, even if I explicitly request US units. This means we will have inconsistent units here. I'm wondering if we should convert them to US in this case. The NWS API is full of these inconsistencies.

@MatthewFlamm MatthewFlamm merged commit c934ee5 into MatthewFlamm:master Jul 2, 2023
@lymanepp lymanepp deleted the ha-humidity branch July 2, 2023 17:22
@lymanepp
Copy link
Copy Markdown
Collaborator Author

lymanepp commented Jul 2, 2023

I need to think about this a little bit, but I noticed that the units for dewpoint are always in SI, even if I explicitly request US units. This means we will have inconsistent units here. I'm wondering if we should convert them to US in this case. The NWS API is full of these inconsistencies.

I just noticed your comment. But I already fixed this and created PR #119 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants