Conversation
othmaneechc
changed the title
favyen2
left a comment
favyen2
left a comment
There was a problem hiding this comment.
This is great, thanks for adding this data source! Just had one question about representing this as one item per window vs a couple of alternative approaches.
APatrickJ
left a comment
APatrickJ
left a comment
There was a problem hiding this comment.
Thank you for the contribution! I have a few comments as well.
othmaneechc
force-pushed
the
othmane/firms-datasource
branch
from
d7b646b to
b2bdbf8
Compare
favyen2
left a comment
favyen2
left a comment
There was a problem hiding this comment.
Thanks for the changes! Had two more comments but otherwise it looks good!
| logger.debug("Requesting FIRMS CSV: %s", url) | ||
| response = session.get(url, timeout=self.timeout.total_seconds()) | ||
| response.raise_for_status() | ||
| features.extend(self._parse_csv_features(response.text, item.bbox)) |
There was a problem hiding this comment.
It looks like we aren't using item.source, and item.bbox is redundant since we can use item.geometry.shp.bounds (maybe raise error if item.geometry.projection != WGS84_PROJECTION). So I think it would make sense to use the base Item class instead of defining FIRMSItem. Several data sources use Item directly and others only define subclasses when it's needed to store additional information.
| assert groups[0][0][0].name == groups[1][0][0].name | ||
|
|
||
|
|
||
| def test_get_items_splits_spatial_bins() -> None: |
There was a problem hiding this comment.
I was expecting it to select all the grid cells (spatial bins) intersecting the window rather than snapping to one of them. Would this cause it to miss the wildfire detections that intersect the window but fall into the other spatial bin?
3 participants
Added FIRMS data source implementation for wildfire detection.