Skip to content

Integration Test fixes - #314

Open
yuvraajnarula wants to merge 12 commits into
openclimatefix:mainfrom
yuvraajnarula:feat/292-integration-test-fix
Open

Integration Test fixes#314
yuvraajnarula wants to merge 12 commits into
openclimatefix:mainfrom
yuvraajnarula:feat/292-integration-test-fix

Conversation

@yuvraajnarula

@yuvraajnarula yuvraajnarula commented Sep 13, 2025

Copy link
Copy Markdown
Contributor

Pull Request

Description

These changes fix multiple errors observed in unit tests and improve PV data handling:

Issues fixed:

  • Missing pv.netcdf file in the repo.
  • KeyError: 'kwp'
  • Row-by-row iteration on large Parquet files was slow.
  • ValueError: You are trying to merge on datetime64[ns] and datetime64[ns, UTC] columns
  • KeyError: 'generation_wh'
  • Metadata CSV inconsistencies.

Improvements in get_pv_metadata and get_pv_truth:

  1. Column fixes:

    • Renamed kWpcapacity for consistent naming.
    • Corrected kwp case issues.
  2. get_pv_metadata:

    • Clean join with pv_id.
    • Standardized metadata column names (latitude, longitude, capacity).
  3. get_pv_truth:

    • Uses Parquet files instead of NetCDF.
    • Selects by ss_id and datetime_GMT.
    • Converts generation values to kWh.
    • Timestamp columns are timezone-aware (UTC) to allow safe merges.
    • Corrects PV generation column name (generation_Wh) to prevent KeyError.
    • Ensures vectorized expansion across horizons for prediction.

These changes improve test reliability and performance when working with PV datasets.

Fixes #292

How Has This Been Tested?

  • Local unit tests for get_pv_metadata and get_pv_truth pass successfully.

Checklist

image

@yuvraajnarula

Copy link
Copy Markdown
Contributor Author

@peterdudfield, there are two Parquet folders: 30_minutely and 5_minutely. Do you want me to create two separate test cases for them, or should we handle it via a single test with an argument to choose the folder?

@peterdudfield

Copy link
Copy Markdown
Contributor

one single test, with an arguement sounded good

@yuvraajnarula

Copy link
Copy Markdown
Contributor Author

@peterdudfield
python -m pytest tests/integration/eval/test_pv.py --foldername 5_minutely -v
30_minutely as a default parameter

@peterdudfield

Copy link
Copy Markdown
Contributor

hey @yuvraajnarula did you manage to get the test passing?

@yuvraajnarula

Copy link
Copy Markdown
Contributor Author

Here are my logs. Do u need a --foldername all -v
to test both of them at the same time?
image
@peterdudfield

Comment thread tests/integration/eval/conftest.py
@yuvraajnarula

Copy link
Copy Markdown
Contributor Author

@peterdudfield any updates?

@@ -0,0 +1,9 @@
import pytest

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.

And can this test be removed?
Or what does this do?

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.

It shouldnt be in conftest.py

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.

so should this contain the pytest_addoption and pytest_generate_tests hooks that enable the --foldername command-line option, not an actual test function?

@yuvraajnarula yuvraajnarula Nov 10, 2025

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.

I have commented down the previous implementation and worked around pytest_addoption and pytest_generate_tests. So sorry for my confusion. Let me know if this is more cleaner and suitable for the PR.

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.

Fix intergrations test

2 participants