Skip to content

Nl blend region#166

Open
PavanRaghavendraKulkarni wants to merge 21 commits intomainfrom
nl_blend_region
Open

Nl blend region#166
PavanRaghavendraKulkarni wants to merge 21 commits intomainfrom
nl_blend_region

Conversation

@PavanRaghavendraKulkarni
Copy link
Copy Markdown
Contributor

@PavanRaghavendraKulkarni PavanRaghavendraKulkarni commented May 5, 2026

Pull Request

Description

This PR introduces regional forecasting to the NL blend application. It updates the orchestration logic to selectively process regional locations (filtering out irrelevant sites by matching names starting with nl_) and applies a distinct set of candidate models for generating regional blend weights.

Key changes:

  • Extracted core weight computation logic in weights.py and added a new get_regional_blend_weights function that uses NL_REGIONAL_CANDIDATE_MODELS.
  • Updated _run_blend_pass in app.py to support a use_regional_weights flag, ensuring the correct weight retrieval function is used based on whether the location is national or regional.
  • Implemented location filtering in the main application loop to only process nl_national and regional states`.
  • Added unit tests to verify the filtering logic and ensure correct routing of national vs. regional weight computations.

Fixes #

How Has This Been Tested?

  • Yes

Tests run:

  • Added a new unit test test_run_blend_app_filters_regional_locations in test_nl_app.py which mocks Data Platform dependencies and confirms that only relevant state locations are processed.

  • Verified that national (nl_national) locations correctly use get_blend_weights while regional locations use get_regional_blend_weights.

  • Validated that both the standard blend and adjuster passes are executed correctly (2 calls per valid location) without processing unrelated sites.

  • Yes

Checklist:

  • My code follows OCF's coding style guidelines
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked my code and corrected any misspellings

@PavanRaghavendraKulkarni PavanRaghavendraKulkarni marked this pull request as ready for review May 5, 2026 11:27
NL_NATIONAL_CANDIDATE_MODELS: list[str] = _cfg.national_candidate_models
NL_REGIONAL_CANDIDATE_MODELS: list[str] = _cfg.regional_candidate_models
BLEND_KERNEL: list[float] = _cfg.blend_kernel
MIN_FORECAST_HORIZON: pd.Timedelta = _cfg.min_forecast_horizon
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

for me this are constants, there are parameters, and therefore should be in lowercase.

I'd move these _cfg = load_blend_config() to within the function, or pass a config into the functions.
This will be important when the config is no longer just for NL, but for NL or Adani, or other country's e.t.c.

You could do this in a separate PR I think

logger = logging.getLogger("blend_app")

# location_type key that identifies the national location in the DP location map.
NL_NATIONAL_LOCATION_KEY = "nl_national"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

could you add a TODO here, this should come from the config at some point

loc.location_name: loc.location_uuid
for loc in dp_locations
if loc.location_name != NL_NATIONAL_LOCATION_KEY
and loc.location_type == dp.LocationType.STATE
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I've move dp.LocationType.STATE into the config, as for other countries, models this wont be the case

Comment thread tests/unit/test_nl_app.py
geometry_wkt="POINT(0 0)",
location_type=dp.LocationType.SITE,
effective_capacity_watts=1_000,
valid_from_utc=datetime(2020, 1, 1, tzinfo=UTC),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why has this be added?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That block of code is required to seed the test database before the application runs. because initially dp container is empty

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yea, but why a SITE? NL doesnt use sites

Comment thread tests/unit/test_nl_app.py
assert deps["get_blend_weights"].call_count == 2
assert deps["get_regional_blend_weights"].call_count == 2
assert deps["get_blend_forecast_values_latest"].call_count == 4
assert deps["_save_forecasts"].call_count == 4
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

good comments

Comment thread tests/conftest.py


@pytest.fixture(scope="module")
def dp_address():
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

how come you need this now? I thought you mocked most things out?

Copy link
Copy Markdown
Contributor

@peterdudfield peterdudfield left a comment

Choose a reason for hiding this comment

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

Please see comments

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.

2 participants