GNOME Bugzilla – Bug 792328
API change: Split off cities from weather stations
Last modified: 2018-03-09 11:26:35 UTC
In commit 6d2b55b6711ebe0beb8ab2698d9f7eb6c8ed044b, we started looking for close weather stations when a city didn't have any defined. I think that we should bring airports/weather stations at the same level as cities, and simply select the closest airport to each city, instead of: - duplicating weather station definitions (within the same country at least, there's a few cases of cross-border airports in mainland Europe and Ireland) - spending time looking at which airport is available for which major city, or whether one airport or another is closer to a city. We'd just define all the ones we know about and have weather stations, and be done with it. This second part of the work is what I spent most of the time doing when I started cleaning up the France definitions late last month.
I like this idea, it makes sense and it will help with software like Clocks that cares more about cities than weather stations. It's also worth saying that only the METAR backend needs weather stations (ICAO codes), as the yr.no and owm backends use geo coordinates instead. One question I have is, should airports be at the same level as cities or directly inside countries? That is, can you be matched to an airport in a different state (in US/Canada)? Also, I will gladly take patches for this once 3.27.4 is out.
I think we could/should completely ignore countries, and look cross-country-and-state-border for the closest airports, although I'd want to check the performance impact of doing that. Hopefully it's minimal as long as the algorithm isn't too stupid.
Created attachment 369438 [details] [review] locations: Remove <province> sub-division It's only used in the Italy, and matches to the description of a "state" and the GWEATHER_LOCATION_ADM1 level: @GWEATHER_LOCATION_ADM1: A location representing a "first-level administrative division"; ie, a state, province, or similar division. This removes the need for a least one fips-code to be attached to a state (Belgium uses multiple ones for some states).
Created attachment 369439 [details] [review] tests: Don't require weather stations to have timezones
Created attachment 369440 [details] [review] GWeatherLocation: Don't try to get distance to elements without coords Skip over elements that don't have valid coordinates when trying to find the closest weather station, as that would have returned incorrect values.
Created attachment 369441 [details] [review] tests: Only check city to weather station distances And not to country or state. As those don't have coordinates anyway, they would have caused warnings and returned invalid values.
Created attachment 369442 [details] [review] GWeatherLocation: Warn for locations without coords Warn when trying to get distance between 2 locations and one of them has no coordinates.
Created attachment 369443 [details] [review] data: Add temporary script to move <location>s Move <location>s to the administrative division above, be it <state> or <country> so that weather stations can be shared between multiple cities without the need to duplicate them. We can then use this to list major cities, and available airports/weather stations, without having to associate them too closely.
Created attachment 369444 [details] [review] Locations: Move weather station locations to <state> or <country> And use this to find the closest weather station for a particular city.
Created attachment 369445 [details] [review] Locations: Disallow <location> in city Weather stations need to be attached to a state or country now.
Created attachment 369446 [details] [review] GWeatherLocation: Allow disabling automatic weather station creation Add an environment variable to disable the copy of weather stations to attach them to cities. This makes it easier to find duplicate weather stations.
Created attachment 369447 [details] [review] tests: Rename "bad" duplicate weather station test Rename the test as it's checking whether weather stations are badly duplicated, not whether they're unique in the state or country. Duplicate weather stations are allowed across state and country borders.
Created attachment 369448 [details] [review] GWeatherLocation: Add helper to get stringified level Add gweather_location_level_to_string() to make printing the level of a location easier, and ease debugging.
Created attachment 369449 [details] [review] tests: Check for duplicate weather stations in country or state Check for duplicate weather stations in country or state, so as to avoid having duplicates listed at the same level.
Created attachment 369450 [details] [review] Locations: Remove duplicate <station>s within the same state or country.
I moved the weather stations to either the state, or the country level, and removed all the duplicates. The tests pass, but that's as far as I went with this. Things that we might want to do: - remove provinces/states in Italy. It's the only European country to have those, and they're of limited use, and would probably hamper things move than help. - revert the scripts I added, now that they've been applied. I think this should already make adding new stations, or curating cities a whole lot easier, as they can now be considered independently of each other.
I've pushed this to https://gitlab.gnome.org/GNOME/libgweather/tree/wip/flatten-stations for now, for the purpose of running the CI. Let me know what you think.
Review of attachment 369438 [details] [review]: The difference between province and state is that the name of the state is shown by default, and the province is not. I agree that provinces (ADM2) should be removed - they were added experimentally when the yr.no used city names instead of geo coordinates - but you should remove states from Italy too, or the display names will look ugly.
Review of attachment 369439 [details] [review]: OK
Review of attachment 369440 [details] [review]: Sure
Review of attachment 369441 [details] [review]: Ok
Review of attachment 369442 [details] [review]: Ok
Review of attachment 369443 [details] [review]: I don't know xsl that well, but I trust you it does what it says
Review of attachment 369443 [details] [review]: I don't know XSLT that well either, but it should do what it says. The 'sed' are to clean up changes xsltproc does which we don't want in the final change. The results of running that script are in the humongous patch later in the series.
Review of attachment 369444 [details] [review]: Not reviewing this one...
Review of attachment 369445 [details] [review]: OK
Review of attachment 369447 [details] [review]: Ok
Review of attachment 369448 [details] [review]: Ok
Review of attachment 369449 [details] [review]: Ok
Review of attachment 369450 [details] [review]: Ok
Review of attachment 369446 [details] [review]: Do we still need to attach weather stations to cities at all?
Review of attachment 369446 [details] [review]: If we want to avoid breaking applications (or API expectations), we do. I don't know to what degree applications have used, and expect, this hierarchy, versus using canned widgets. This avoids searching for the closest weather station by hand in the siblings of the <city> as well, though it would be fast in most cases. At a minimum we should add a helper to find the closest weather station.
I see. That's fine for now, we should revisit for the next cycle though. Also, we're a little behind schedule for 3.27.91/92, would you mind doing the release when you're done? Thanks!
(In reply to Bastien Nocera from comment #32) > Review of attachment 369446 [details] [review] [review]: > > If we want to avoid breaking applications (or API expectations), we do. I > don't know to what degree applications have used, and expect, this > hierarchy, versus using canned widgets. > > This avoids searching for the closest weather station by hand in the > siblings of the <city> as well, though it would be fast in most cases. At a > minimum we should add a helper to find the closest weather station. I filed https://gitlab.gnome.org/GNOME/libgweather/issues/2 about this. (In reply to Giovanni Campagna from comment #33) > I see. That's fine for now, we should revisit for the next cycle though. > > Also, we're a little behind schedule for 3.27.91/92, would you mind doing > the release when you're done? It's too late to put that in 3.28, release day is next Monday. I'll branch and push revised versions of all the above which are now in the wip/flatten-stations branch on gitlab. And I'll do a release for 3.28 from the current master.
Created attachment 369485 [details] [review] locations: Remove <province> sub-division It's only used in the Italy, and matches to the description of a "state" and the GWEATHER_LOCATION_ADM1 level: @GWEATHER_LOCATION_ADM1: A location representing a "first-level administrative division"; ie, a state, province, or similar division. The only difference between ADM1 and ADM2 levels were that ADM1 levels are shown in the location entry, whereas ADM2 levels are not.
Created attachment 369486 [details] [review] GWeatherLocation: Remove ADM2 sub-division Now that nothing uses it, remove ADM2.
Created attachment 369487 [details] [review] tests: Don't require weather stations to have timezones
Created attachment 369488 [details] [review] GWeatherLocation: Don't try to get distance to elements without coords Skip over elements that don't have valid coordinates when trying to find the closest weather station, as that would have returned incorrect values.
Created attachment 369489 [details] [review] tests: Only check city to weather station distances And not to country or state. As those don't have coordinates anyway, they would have caused warnings and returned invalid values.
Created attachment 369490 [details] [review] GWeatherLocation: Warn for locations without coords Warn when trying to get distance between 2 locations and one of them has no coordinates.
Created attachment 369491 [details] [review] data: Add temporary script to move <location>s Move <location>s to the administrative division above, be it <state> or <country> so that weather stations can be shared between multiple cities without the need to duplicate them. We can then use this to list major cities, and available airports/weather stations, without having to associate them too closely.
Created attachment 369492 [details] [review] Locations: Move weather station locations to <state> or <country> And use this to find the closest weather station for a particular city.
Created attachment 369493 [details] [review] Revert "data: Add temporary script to move <location>s" This reverts commit 5583da22fbe1d56e0280cdf37deeed5b146574e9. Now that the script has been used, no need to keep it around.
Created attachment 369494 [details] [review] Locations: Disallow <location> in city Weather stations need to be attached to a state or country now.
Created attachment 369495 [details] [review] GWeatherLocation: Allow disabling automatic weather station creation Add an environment variable to disable the copy of weather stations to attach them to cities. This makes it easier to find duplicate weather stations.
Created attachment 369496 [details] [review] tests: Rename "bad" duplicate weather station test Rename the test as it's checking whether weather stations are badly duplicated, not whether they're unique in the state or country. Duplicate weather stations are allowed across state and country borders.
Created attachment 369497 [details] [review] GWeatherLocation: Add helper to get stringified level Add gweather_location_level_to_string() to make printing the level of a location easier, and ease debugging.
Created attachment 369498 [details] [review] tests: Check for duplicate weather stations in country or state Check for duplicate weather stations in country or state, so as to avoid having duplicates listed at the same level.
Created attachment 369499 [details] [review] Locations: Remove duplicate <station>s within the same state or country.
Attachment 369485 [details] pushed as e3a1277 - locations: Remove <province> sub-division Attachment 369486 [details] pushed as 4e7a3a9 - GWeatherLocation: Remove ADM2 sub-division Attachment 369487 [details] pushed as 6346587 - tests: Don't require weather stations to have timezones Attachment 369488 [details] pushed as ff5b56a - GWeatherLocation: Don't try to get distance to elements without coords Attachment 369489 [details] pushed as 5672119 - tests: Only check city to weather station distances Attachment 369490 [details] pushed as 7a313a8 - GWeatherLocation: Warn for locations without coords Attachment 369491 [details] pushed as 8ff27e6 - data: Add temporary script to move <location>s Attachment 369492 [details] pushed as d768267 - Locations: Move weather station locations to <state> or <country> Attachment 369493 [details] pushed as 8c65b68 - Revert "data: Add temporary script to move <location>s" Attachment 369494 [details] pushed as 82d640f - Locations: Disallow <location> in city Attachment 369495 [details] pushed as cc7384f - GWeatherLocation: Allow disabling automatic weather station creation Attachment 369496 [details] pushed as 157d2dc - tests: Rename "bad" duplicate weather station test Attachment 369497 [details] pushed as 622c4db - GWeatherLocation: Add helper to get stringified level Attachment 369498 [details] pushed as 58ecb61 - tests: Check for duplicate weather stations in country or state Attachment 369499 [details] pushed as 97ecaeb - Locations: Remove duplicate <station>s