search

nasa / opera-sds-pcm

Observational Products for End-Users from Remote Sensing Analysis (OPERA)
Apache License 2.0
16 stars 12 forks source link

[Bug]: DSWx-S1 trigger logic never applies grace period #886

Closed sjlewis-jpl closed 2 months ago

sjlewis-jpl commented 3 months ago

Checked for duplicates

No - I haven't checked

Describe the bug

I believe I have found a bug (or bugs) in the DSWx-S1 trigger logic, where it is not ever applying the grace period. It’s also logging TileSets with too few bursts as being within the grace period.

First, I think this line is why the grace period isn’t being applied: data_subscriber/rtc/evaluator.py#L140. There’s an extra continue statement such that, if the min_num_bursts is defined, it will skip the rest of the loop where the grace period logic is kept. And in the FWD mode logs, I am not seeing any evidence that the grace period is being applied, when it really should be. The log lines (L150 and L154) never appear in the logs.

Second, TileSets that are skipped due to having too few bursts (detected in the lines just preceeding L140 with the erroneous continue statement), are being reported out as being within the grace period (L158).

The products being added to product_burstset_index_to_skip_processing is capturing those with too few bursts. If the first bug is fixed, it will also capture those legitimately in the grace period, and log both out identically.

What did you expect?

I expected the trigger logic to apply the grace period. I expected the logs to accurately record the reasons why TileSets are being skipped.

Reproducible steps

1. Run in FWD mode
2. Look at the logs for the rtc-query job
3. Try to find evidence that the grace period is actually being applied

Environment

- R3.0.0-RC.5.0
- INT-FWD
riverma commented 3 months ago

Thanks for deep diving this @sjlewis-jpl. Yeah, I think L118 through L161 should be taken out of main and placed in a modular function that can be tested for grace period logic within https://github.com/nasa/opera-sds-pcm/blob/develop/tests/unit/data_subscriber/test_evaluator_core.py

The grace period logic isn’t being tested for in unit tests from what I could find.

chrisjrd commented 3 months ago

In a meeting today, we decided to keep the mutually exclusive behavior of min_num_bursts and coverage_percentage, but update PCM to also apply the grace period logic to burst sets when min_num_bursts is specified.

riverma commented 3 months ago

In a meeting today, we decided to keep the mutually exclusive behavior of min_num_bursts and coverage_percentage, but update PCM to also apply the grace period logic to burst sets when min_num_bursts is specified.

Thanks @chrisjrd - a unit test to prove out the grace period logic is working (and not breaking upon future changes) would be great too.

sjlewis-jpl commented 3 months ago

In a meeting today, we decided to keep the mutually exclusive behavior of min_num_bursts and coverage_percentage, but update PCM to also apply the grace period logic to burst sets when min_num_bursts is specified.

Hi @chrisjrd - is there a ticket or other documentation of a request to remove the grace period logic? Or to exclude it from consideration when using the minimum burst count filter? I'm trying to track down how this miscommunication happened, since from my perspective the grace period was never supposed to be removed or excluded.