fix: Make get_multiple_* methods pass tests by not halting for one bad input
#128
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Instead of halting/raising exceptions, the get_multiple_* methods now continue to work when encountering bad inputs. This works by catching exceptions raised by its respective get_*_air methods. Why? Because get_multiple_* (I think) is supposed to be a convenient wrapper for multiple inputs at once. It's not really convenient to break the entire execution for one bad input. For consistency, the output dataframe will have exactly one row per one input, where bad inputs get all-nan rows apart from its 'city' or 'lat'-'lon' columns.
|
@lahdjirayhan Absolutely! This is something that definitely occurred to me before, but I didn't give it enough thought. The use case that you mentioned of dozens of cities (or more) is certainly one that would be annoying without this. I think it's a badly needed improvement. Excited that the tests are helping so much! Thanks again for making such a robust set of tests. |
Milind220
reviewed
Apr 24, 2022
| self.get_coordinate_air(loc[0], loc[1], df=df, params=params) | ||
| ) | ||
| except Exception: | ||
| # NOTE: If we have custom exception we can catch it instead. |
There was a problem hiding this comment.
Hmm overall I do think we should have custom exceptions.
However, it's a good idea to add custom exceptions for the entirety of Ozone in one go (I'm sure there are other sections that need them). This way we can come up with a consistent set of exceptions for the package.
Milind220
reviewed
Apr 24, 2022
| # The third row should have the "nonexistent city name" intact ... | ||
| assert result.at[2, "city"] == "a definitely nonexistent city" | ||
|
|
||
| # ... and nothing else. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As laid out by the tests,
get_multiple_*_airmethods shouldn't halt when there is one bad input. One bad input (which in turn will make backend return error/bad response, which will make an exception raised in our part) shouldn't halt the execution of other, likely good, inputs. This is not convenient. For what it's worth, I've always treatedget_multiple_*methods as convenience wrappers for their singleget_*_aircounterparts (otherwise I could just loop them myself).The example in my mind is this: you are mass-collecting data for a hundred cities. One city turns out to not have AQI station and subsequently raise exception (you likely won't know this in advance). It will be annoying to have the other 99 cities halted.
I realize that this will make an odd difference between e.g.
get_city_air(raises exception on bad response) andget_multiple_cities_air(returns nan row on bad response). I don't have really much to say either supporting or against.Is this a good or common enough use case? Should I implement things differently? I'm curious to hear more.