Skip to content

Extending tide prediction to Landsat and NISAR & adding S2C#54

Open
ifenni wants to merge 12 commits into
OPERA-Cal-Val:mainfrom
ifenni:main
Open

Extending tide prediction to Landsat and NISAR & adding S2C#54
ifenni wants to merge 12 commits into
OPERA-Cal-Val:mainfrom
ifenni:main

Conversation

@ifenni

@ifenni ifenni commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

The tide prediction feature (-t flag) previously worked only for Sentinel-1/2, since ESA provides exact overpass times.
This PR extends the same NOAA tide prediction feature to Landsat 8/9 and NISAR overpasses.

** What's new:

  • Landsat: USGS provides dates only, so overpass times are estimated from orbital specs (Landsat crosses the equator at 10:12 AM local solar time). Estimated UTC times are used to query NOAA and interpolate tide values for each overpass.
  • NISAR: JPL's observation-plan KMZ also provides dates only. Overpass times are estimated from NISAR's nodal crossings (6 AM local for ascending, 6 PM for descending), using the pass direction included in the KMZ.
  • Consistent output: All three satellite families now display tide predictions in the same format (height(phase-direction), e.g. 1.23(H-rising)) both in the text table and in map popups.
  • Estimation accuracy: ±20–40 min for Landsat/NISAR, translating to ~±0.3 m tide uncertainty. We think that this is sufficient for operational planning (tides change ~0.1–0.3 m/hr).

** Usage, unchanged from for only Sentinel :

next-pass -b 43.219 45.15 -124.398 -122.2 -t

In this PR, we also fix an issue with the calculation of the percentage of overlap between the overpass and AOI for Landsat. The problem was that when there are multiple rows that intersect with the AOI for the same path/direction/mission combination, the code was adding the intersection percentages together instead of merging the geometries first. The applied change is moving from overlap_pct += intersection_pct (adding percentages) to calculating the intersection once from the geometry of all rows.

@ifenni ifenni requested review from cmspeed and ehavazli July 2, 2026 21:50
@ehavazli

ehavazli commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Code Review Findings

Found several issues that need attention:

Critical: UTC hour rollover bug (P1)

Affects: utils/nisar_pass.py:82-98 and utils/landsat_pass.py:80-96

Both files compute UTC hours with utc_hour % 24 then use date_obj.replace(hour=..., tzinfo=timezone.utc). This keeps the local calendar date even when UTC conversion should roll to the previous day.

Impact:

  • Western hemisphere descending NISAR passes show dates one day early
  • Those wrong dates propagate to NOAA tide lookups (fetching tide for wrong day)
  • Far-eastern AOIs with Landsat have the same issue

Fix: Build result from datetime.combine(date_obj, time.min, tz=timezone.utc) + timedelta(hours=utc_hour) instead of using .replace() after modulo.


Behavioral issue: Tide filtering drops passes (P2)

Affects: utils/landsat_pass.py:567-592, utils/sentinel_pass.py:370-387, utils/nisar_pass.py

When --tide is requested, passes outside NOAA's 60-day prediction window are dropped entirely instead of being shown with unavailable tide data.

get_tide_info_batch() already returns None for unavailable dates - these passes should remain visible with tide: N/A rather than disappearing from the output.


Test runner crash

Affects: tests/test_timezone_validation.py:167-187

The __main__ block treats test functions as returning booleans, but they return None. This causes sum(results) to crash.

Fix:

tests = [test_utc_datetime, test_non_utc_datetime, test_naive_datetime, test_datetime_list]
results = []
for test in tests:
    try:
        test()
        results.append(True)
    except AssertionError as e:
        print(f"FAILED: {test.__name__}: {e}")
        results.append(False)

@ehavazli ehavazli left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see my comment above

@ifenni

ifenni commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

** Issue 1 fixed with d116594 and 1dc5a77. The solution is to use datetime.combine() + timedelta to let the arithmetic naturally roll the date:
base = datetime.combine(date_obj.date(), time.min, tzinfo=timezone.utc)
return base + timedelta(hours=raw_utc_hour)
This makes the day rollover implicit and correct. Example: timedelta(hours=26) naturally moves to the next day, timedelta(hours=-4) naturally moves to the previous day.

** Issue 2: The concern is that if we add tide prediction for all overpasses including those beyond the 60 days filter, the results will be hard to read and interpret. Therefore, we keep the current filtering behavior (hide dates beyond 60 days entirely), but add a summary line at the end to let users know about the hidden future passes. The user can always re-run without -t to check overpasses beyond 60 days if needed. Note that the 60 days filtering applies only to Landsat and NISAR.

** issue 3: Fixed test runner crash by wrapping test function calls in try/except blocks to explicitly track pass/fail status (True/False) instead of relying on implicit None returns that caused TypeError
in sum() and made all() always evaluate to False.

@ifenni ifenni requested a review from ehavazli July 8, 2026 00:25
@ifenni ifenni changed the title Extending tide prediction to Landsat and NISAR Extending tide prediction to Landsat and NISAR & adding S2C Jul 8, 2026
@ehavazli

ehavazli commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Here are some recommendations to improve maintainability:

1. Extract duplicate 60-day filtering logic

The same filtering logic appears in both utils/landsat_pass.py (lines 821-857) and utils/nisar_pass.py (lines 1335-1367). Consider extracting to a shared function:

def filter_future_dates(dates, tide_data, max_days=60):
    """Filter dates/tides to only those within max_days from now."""
    # Shared implementation
    pass

This would reduce duplication and make the 60-day limit easier to adjust in the future.

2. Convert magic numbers to named constants

Several hardcoded values should be module-level constants:

# utils/landsat_pass.py:636
LANDSAT_EQUATORIAL_CROSSING_HOUR = 10.2  # 10:12 AM local solar time

# utils/nisar_pass.py:1041-1044
NISAR_ASCENDING_CROSSING_HOUR = 6.0   # 6:00 AM local solar time
NISAR_DESCENDING_CROSSING_HOUR = 18.0  # 6:00 PM local solar time

This improves readability and makes the orbital specifications more discoverable.

3. Add accuracy disclaimer to CLI output

The overpass time estimates have ±15-40 minute accuracy (documented in docstrings), but users don't see this. Consider adding a note to the table output:

Note: Overpass times are estimates based on orbital specifications (±20-40 minutes accuracy)

This sets proper expectations for the tide predictions.

4. Document pass count filtering behavior

When --tide is used, the number of displayed passes is filtered to 60 days, but without --tide users see 5+ passes. This could be confusing. Consider either:

  • Mentioning this in the --tide help text
  • Adding it to the user-facing note that already mentions filtered passes
  • Making the filtering independent of the tide flag

Example help text:

--tide: Include NOAA tide predictions (limits display to passes within 60 days)

@cmspeed

cmspeed commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

I tried the test example command provided above: next-pass -b 43.219 45.15 -124.398 -122.2 -t

I see that the next_pass printout populates tide predictions for all sensors and looks great.

However, I tried another AOI offshore Louisiana: next-pass -b 29 32 -94 -89 -t

It seems like there are multiple tide gauges intersected by the satellite acquisitions (see the first screenshot), but I noticed that the tide predictions are N/A for every sensor except Sentinel-1.

Screenshot 2026-07-08 at 12 04 03 PM image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants