TopoSplit - switch to bilinear interpolation and fix HRRR overcast zero-dropouts

[jmichellehu]

Summary of changes

• Switch default interpolation method from cubic to bilinear. This avoids introducing values outside the source data's range (undershoot/overshoot).
• Remove minimum value gate logic (AND) on incident, direct and diffuse components. One failing condition zeroed the pixel, resulting in erroneous zero dropouts in incident solar radiation during overcast daylight conditions when direct values did not meet the minimum value threshold. Replace with single gate on incident radiation (DSWRF).
• Clamp direct and diffuse components that do not meet minimum value to zero.
• Implement ghi_vis minimum value threshold to guard against divide-by-zero errors in k value (diffuse fraction) calculations. Per-component clamping increases this risk.
• Expand tests to cover near-threshold, below-threshold, mostly-direct/diffuse cases that reflect updated clamping and guard logic.
• Regenerate gold reference files (hrrr_solar, net_solar_hrrr*, solar_k, thermal_hrrr*) to reflect preceding interpolation and threshold changes.

[Copilot]

🟡 Changes recommended

Unresolved critical radiation-validity and moderate fallback findings block approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Updates HRRR solar processing to use bilinear interpolation and adjust threshold handling for interpolated components.

Changes:

• Sets the configured GDAL interpolation default to bilinear.
• Updates solar component threshold processing.
• Adds regression tests for threshold and negative-component behavior.

File summaries

File	Reviewed changes
smrf/tests/distribute/test_solar_hrrr.py	Adds solar behavior coverage; the threshold test no longer preserves mixed per-pixel coverage.
smrf/framework/CoreConfig.ini	Sets bilinear interpolation as the configured default; the runtime fallback remains inconsistent.
smrf/envphys/solar/toposplit.pyx	Updates solar processing conditions; negative components can produce invalid radiation, and the new comment contains a duplicate is.

Review details

Suppressed comments (1)

smrf/tests/distribute/test_solar_hrrr.py:143

• This test now sets DSWRF below threshold for every pixel and asserts every output is zero, so it no longer covers the per-pixel case where a low-DSWRF cell is adjacent to a valid cell. The previous test exercised that mixed mask; keep a mixed input and assert only the low cell is zero to catch regressions that incorrectly clear the whole grid.

        data = {
            SolarHRRR.DSWRF: np.full_like(SKY_VIEW_FACTOR_MOCK, 0.0),
            SolarHRRR.VBDSF: np.full_like(SKY_VIEW_FACTOR_MOCK, 6.0),
            SolarHRRR.VDDSF: np.full_like(SKY_VIEW_FACTOR_MOCK, 5.0),

• Files reviewed: 3/3 changed files
• Comments generated: 3
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟢 Approval recommended

All changes are reviewed; only a non-blocking documentation nit remains.

Review details

Suppressed comments (1)

smrf/tests/distribute/test_solar_hrrr.py:174

• This sentence is grammatically incorrect: the article “a” is followed by plural “conditions.” Use the singular “condition” (and drop “legitimately”) so the test documentation is clear.

        k is fully diffuse (1.0) and dni/direct are 0.

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[jmichellehu]

Wasn't sure how we want to handle failed gold tests @jomey. Figured it'd be best to regenerate solar hrrr affected files and push up. Feedback welcome

[jomey]

Quick recapture from the meeting with Michelle to keep Matt and Alvaro in the loop:

• Remove intermediate variables and clamp invalid values directly with the input arrays
• Make DSWRF the sole condition check whether to do the calculation. If a value is bigger than 1, there must be either diffuse or direct in there.
• Add test that verifies we won't do calculations for values less than 1 to prevent massive K values when there are interpolation values between 0 and 1

[jmichellehu]

Made a few changes upon review

• Remove intermediate variables and clamp invalid values directly with the input arrays

Input arrays were read-only so direct assignment did not work, stuck to the scalar approach

• Make DSWRF the sole condition check whether to do the calculation. If a value is bigger than 1, there must be either diffuse or direct in there.

This was reorganized such that DSWRF condition is the prime guard, but without the ghi_vis guard, the divide-by-zero risk in k calculations was made more likely when both direct_normal and diffuse_horizontal are clamped to 0. Decided to reimplement the ghi_vis guard

• Add test that verifies we won't do calculations for values less than 1 to prevent massive K values when there are interpolation values between 0 and 1

Instead of a 1 Wm-2 threshold, I set it to 0.1 Wm-2 since the smaller fractional values are the ones we are concerned about in the first place. This way, k isn't too distorted from small values of VBDSF or VDDSF
As an example, let's assume VDDSF = 0.9 and VBDSF = 5:
Given a 1 Wm-2 threshold for each component, VDDSF is clamped to 0 and k = 0
Given a lower threshold of 0.1 Wm-2, the VDDSF component is retained and resulting k = 0.155

[Copilot]

Copilot review overview

🟡 Changes recommended

Add test coverage that directly exercises the new bilinear default mapping.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Add test coverage for default resampling algorithm mapping · New

[jmichellehu]

Gold-NetCDF-Compare.ipynb outputs comparing topocalc v20260526 (cubic interp + 3 condition gate) with proposed changes here:

hrrr_solar

net_solar_hrrr_albedo

net_solar_hrrr_net_solar

net_solar_hrrr_vegetation_net_solar

solar_k_solar_k

thermal_hrrr_thermal

thermal_hrrr_vegetation_thermal

[jomey]

• Add test that verifies we won't do calculations for values less than 1 to prevent massive K values when there are interpolation values between 0 and 1

Instead of a 1 Wm-2 threshold, I set it to 0.1 Wm-2 since the smaller fractional values are the ones we are concerned about in the first place. This way, k isn't too distorted from small values of VBDSF or VDDSF As an example, let's assume VDDSF = 0.9 and VBDSF = 5: Given a 1 Wm-2 threshold for each component, VDDSF is clamped to 0 and k = 0 Given a lower threshold of 0.1 Wm-2, the VDDSF component is retained and resulting k = 0.155

My vote is to use the 1 Wm-2 threshold throughout all of the checks and clamping and change/update if needed. It was introduced to account for interpolation artifacts and now that we changing the logic, there is no need to hang on to this. With the new logic, we should also prevent any combination of pixels that are not DSWRF > ghi_vis > 0. I will add some suggestions on where, how I could see this fit.

Since our logic is also more complex now, I vote to update the tests to use more physical meaningful logic. I will provide those in line too.

[jomey]

Lastly, can we also update the title and description of this PR. Right now it hides a very key change and would be hard to find in the future if we need to get back to this.

[jmichellehu]

Two more notes

• gold files did not require regeneration (preceding plots stand) as there were no pixels within the threshold change region (0.01–1.0).
• users who installed an editable version of smrf will need to recompile (python setup.py build_ext --inplace), while other users will need to explicitly reinstall (pip install . or pip install --upgrade .) within the repo root

I think this is ready @jomey!

[jomey]

Did you change the SolarHRRR.MIN_RADIATION value locally and forgot to commit?

Right now we would still use the old 1 Wm-2 since this is how it is initialized.

[jomey]

* users who installed an editable version of smrf will need to recompile (`python setup.py build_ext --inplace`), while other users will need to explicitly reinstall (`pip install .` or `pip install --upgrade .`) within the repo root

Also noting that there is the Makefile, which has exactly those targets defined.

• build_extensions
• install

[jmichellehu]

Did you change the SolarHRRR.MIN_RADIATION value locally and forgot to commit?

Right now we would still use the old 1 Wm-2 since this is how it is initialized.

Nope, missed that, fixed now @jomey