Feature/asos reaper#7
Conversation
Co-authored-by: Copilot <copilot@github.com>
cheginit
left a comment
cheginit
left a comment
There was a problem hiding this comment.
I added a few comments.
| def _build_url(self) -> str: | ||
| """Build the IEM ASOS request URL with query parameters.""" | ||
| params = [ | ||
| ("network", self.network), | ||
| ("year1", self.start_date.year), | ||
| ("month1", self.start_date.month), | ||
| ("day1", self.start_date.day), | ||
| ("year2", self.end_date.year), | ||
| ("month2", self.end_date.month), | ||
| ("day2", self.end_date.day), | ||
| ("format", "comma"), | ||
| ("latlon", "yes"), | ||
| ] | ||
| for var in self.data_vars: | ||
| params.append(("data", var)) | ||
| return f"{BASE_URL}?{urlencode(params)}" |
There was a problem hiding this comment.
Please rewrite with doseq:
def _build_url(self) -> str:
params = {
"network": self.network,
"year1": self.start_date.year,
"month1": self.start_date.month,
"day1": self.start_date.day,
"year2": self.end_date.year,
"month2": self.end_date.month,
"day2": self.end_date.day,
"format": "comma",
"latlon": "yes",
"data": self.data_vars, # list is handled by doseq
}
return f"{BASE_URL}?{urlencode(params, doseq=True)}"I looked into the IEM ASOS backend docs and found a few missing params worth addressing:
tz (bug risk): The API accepts a tz parameter controlling how observation timestamps are presented. Omitting it is risky: a recently patched server bug caused it to default to America/Chicago instead of UTC when tz was absent. Should explicitly pass tz=Etc/UTC (or whatever timezone is intended) rather than rely on server default.
station: The API supports per-station filtering via repeated station= params (e.g. station=DSM&station=MCW), separate from network. If this class intentionally fetches by network only, that's fine but worth a comment.
report_type: Omitting this returns all report types (MADIS HFMETAR/5-min, routine hourly, and specials) combined. If the intent is clean hourly obs only, this should be set explicitly.
elev: Minor, but since latlon=yes is already set, elev=yes is available if elevation is useful downstream.
The tz omission is the only one that's a correctness issue; the rest are defaults that may or may not match intent.
| """ | ||
| super().__init__() | ||
| self.state = state | ||
| self.network = "ASOS" if state.upper() == "ASOS" else f"{state.upper()}_ASOS" |
There was a problem hiding this comment.
"ASOS" is not a valid IEM network identifier. Global fetch likely requires station=_ALL; clarify intended behavior.
| df = pd.read_csv(StringIO(text), skiprows=5) | ||
| if df.empty: | ||
| logger.warning(f"ASOS returned no data for network {self.network}") | ||
| return pd.DataFrame() |
There was a problem hiding this comment.
Empty return conflates "no data" with error; caller in _reap cannot distinguish. Consider raising DataNotFoundError or similar instead.
| except Exception as e: | ||
| raise DateRangeError(f"Could not parse date: {e}") from e | ||
|
|
||
| if variable is None: |
There was a problem hiding this comment.
"all" is an IEM magic value; extract as module-level constant (e.g. _IEM_ALL_VARS = "all") so it's not a bare string here and in any future URL building.
| class ASOSReaper(TimeSeriesReaper): | ||
| """Reaper for IEM ASOS data.""" | ||
|
|
||
| timeout: int = 120 |
There was a problem hiding this comment.
Class-level attribute is mutable globally (ASOSReaper.timeout = x affects all instances). Move to __init__ param, if per-instance override is intended, or rename TIMEOUT to signal it's a constant.
2 participants
Adds ASOS reaper