google-research / google-research/flood-forecasting
[Bug] sample_cmal_deterministic ignores negative_sample_handling config and produces negative streamflow values
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 347
- Forks
- 92
- Avg merge
- 8d 7h
- Merged PRs (30d)
- 4
Description
### Description
When a model is configured with `negative_sample_handling: 'clip'`, predictions are expected to enforce physical non-negativity ($Q \ge 0.0$).
While `sample_cmal()` correctly ingests the dataset `Scaler`, calculates the normalized zero threshold, and applies `_handle_negative_values()`, `sample_cmal_deterministic()` omits the `scaler` parameter and bypasses negative sample handling entirely. Consequently, deterministic CMAL forecasts
produce negative streamflow predictions when unscaled.
In `test/test_uncertainty.py:137-144`, this discrepancy was bypassed by explicitly exempting `cmal_deterministic` from the non-negativity check:
```python
if (
negative_sample_handling == 'clip'
and config.head.lower() != 'cmal_deterministic'
):
assert np.allclose(negative_vals, 0.0, atol=1e-6)
```
### Steps to Reproduce
1. Configure a run with head: 'cmal_deterministic' and negative_sample_handling: 'clip'.
2. Generate predictions during low-flow periods where lower quantiles (q = 0.1,q = 0.2) fall below normalized zero (-center/scale).
3. Unscale predictions to physical space (m³/s).
4. Observed: Lower quantiles evaluate to negative streamflow discharge.
5. Expected: Values below normalized zero are clamped, producing non-negative streamflow (≥0.0).
### Affected Files
• googlehydrology/utils/samplingutils.py (lines 378-415)
• test/test_uncertainty.py (lines 137-144)
### Proposed Fix
1. Update sample_cmal_deterministic in googlehydrology/utils/samplingutils.py to accept scaler: Scaler | None = None and execute _handle_negative_values():
```python
def sample_cmal_deterministic(
model: 'BaseModel',
data: dict[str, torch.Tensor],
*,
scaler: Scaler | None = None,
outputs: dict[str, torch.Tensor] | None = None,
) -> dict[str, torch.Tensor]:
# ... generate sample points ...
if scaler is not None and setup.cfg.negative_sample_handling:
normalized_zero = _calc_normalized_zero_thresholds(
scaler=scaler,
targets=setup.cfg.target_variables,
device=setup.device,
dtype=samples[f'y_hat{setup.freq_suffixes[0]}'].dtype,
)
for freq_suffix in setup.freq_suffixes:
key = f'y_hat{freq_suffix}'
samples[key] = _handle_negative_values(
cfg=setup.cfg,
values=samples[key],
sample_values=lambda ids: samples[key][ids],
normalized_zero=normalized_zero,
)
return samples
```
2. Pass scaler through sample_pointpredictions() at samplingutils.py:65.
3. Remove the and config.head.lower() != 'cmal_deterministic' exception from test/test_uncertainty.py:139.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in googlehydrology/utils/samplingutils.py, reading sample_pointpredictions() and sample_cmal_deterministic(), then inspect test/test_uncertainty.py:137-144. Run the uncertainty tests with cmal_deterministic and negative_sample_handling set to clip. Done means the deterministic path receives the scaler, enforces non-negative unscaled streamflow, and passes after the exemption is removed.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python, pytorch
- Domain
- machine-learning, testing-qa
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 78/100