Conversation
ehavazli
left a comment
There was a problem hiding this comment.
Critical Issues - Must Fix Before Merge
1. Breaking API Change - Function Signature
Location: next_pass.py:255
Adding include_hls between compute_tide and products breaks backward compatibility for positional calls.
Fix: Move include_hls after satellites OR make keyword-only:
def run_next_pass(
functionality: str = "both",
compute_cloudiness: bool = False,
compute_tide: bool = False,
products: List[str] | str | None = None,
satellites: List[str] | str | None = None,
*, # Force keyword-only
include_hls: bool = False,
):2. Wrong earthaccess API Parameter
Location: utils/opera_products.py:180
earthaccess 0.17.0 uses granule_name, not granule_ur. Current code fails silently.
Fix:
results = earthaccess.search_data(short_name=collection, granule_name=granule_id)3. Missing Geometry Guard
Location: utils/opera_products.py:422
Fallback search passes bounding_box=None when geometry missing, may crash.
Fix: Skip fallback if no geometry:
if tile_id and start_time != "N/A" and geom:
search_bounds = geom.bounds
hls_results = earthaccess.search_data(...)4. Only First HLS Granule Processed
Location: utils/opera_products.py:394
next() only processes first HLS input. Multiple source granules ignored.
Fix: Handle all or document why first-only correct:
raw_hls_list = [g for g in input_granules if g.startswith("HLS.")]5. Missing Tests
No test coverage for:
- HLS extraction from InputGranules
- Fallback CMR search
- Band mapping logic (S30 vs L30)
- Error cases
Lower Priority (Follow-up OK)
- Sequential API calls may be slow for large result sets (consider parallelization)
- No progress indication during HLS fetching
- "N/A" doesn't distinguish API failure from missing data
|
Thanks for spotting these. I have just pushed changes addressing the following:
|
ehavazli
left a comment
There was a problem hiding this comment.
Thanks for addressing the issues! The fixes look solid.
A few things to consider for later (not blocking this PR):
The integration tests call the live CMR API which might cause CI flakiness. You could skip them in CI with pytest -m "not integration" or mock the earthaccess calls.
fetch_hls_granule_links returns None on errors but [] when there are no results. Works fine but the mixed return types could trip someone up later.
The tile ID extraction uses length check but doesn't validate the format. Something like r'^T\d{2}[A-Z]{3}$' would catch malformed IDs, though the current approach works for real data.
Looks good to merge.
This PR introduces the ability to fetch and append corresponding source HLS (Harmonized Landsat and Sentinel-2) scenes for OPERA products directly into the output
opera_products_metadata.xlsx. By passing a new CLI flag, users can now automatically retrieve the download links for R/G/B/NIR, and Fmask bands associated with the corresponding OPERA granules.Key Changes
Added
--include-hlsflag tonext_pass.pyto optionally trigger the fetching of source HLS scenes.When
--include-hlsis active, the opera_products_metadata.xlsx file will include six new columns:Utilizes the
earthaccesslibrary to fetch HLS data links. Implements a two-tiered search strategy to find the corresponding source scenes:Maps the Near-Infrared (NIR) band based on the specific HLS collection, accounting for naming differences between Sentinel-2 (HLSS30 uses B8A) and Landsat (HLSL30 uses B05).
Test functionality
The following command will return and
opera_products_metadata.xlsxcontaining the corresponding HLS scenes.