update pytests to accuracy=high#594
Conversation
|
need to update version of RADIANCE used by CI. |
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce CI variability in Radiance-based tests by switching some pytest runs to higher radiance tracing accuracy, and updates CI to use a newer Radiance distribution.
Changes:
- Update selected pytest assertions and calls to use
accuracy='high'in 1-axis cumulative-sky tests. - Add
accuracy: highto some test.inifiles and adjust tolerances/expected values for stability. - Update GitHub Actions pytest workflow to install a newer Radiance release and run tests under
xvfb.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_gencumsky.py |
Improves debug output formatting for gencumsky modelchain test. |
tests/test_bifacial_radiance.py |
Uses accuracy='high' for some 1-axis analyses and tweaks tolerances/expected values. |
tests/ini_highAzimuth.ini |
Adds accuracy / debug options intended to reduce variability for the high-azimuth modelchain test. |
tests/ini_gencumsky.ini |
Adds accuracy: high intended to reduce variability for gencumsky modelchain test. |
docs/sphinx/source/whatsnew/pending.rst |
Adds pending release notes documenting the pytest/CI changes. |
.github/workflows/pytest.yaml |
Switches CI Radiance install to a newer distribution and replaces the xvfb action with xvfb-run. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| accuracy: high | ||
| debug: True |
There was a problem hiding this comment.
analysisParamsDict entries like accuracy: high and debug: True added here are not currently consumed by the modelchain path used in test_Radiance_high_azimuth_modelchains (runModelChain calls analysis.analysis(...) / demo.analysis1axis(...) without forwarding these options). This makes the config misleading and won’t actually reduce test variability. Either plumb analysisParamsDict.get('accuracy') (and debug, if desired) through modelchain into the analysis calls, or remove these keys from the ini to avoid implying they have an effect.
| accuracy: high | |
| debug: True |
| sensorsy: 9 | ||
| modWanted: 5 | ||
| rowWanted: 2 | ||
| accuracy: high |
There was a problem hiding this comment.
accuracy: high is added to analysisParamsDict here, but the fixed-tilt cumulative-sky modelchain code path (used by tests/test_gencumsky.py::test_SingleModule_gencumsky_modelchain) does not currently pass an accuracy value into AnalysisObj.analysis(...), so this setting has no effect. Consider updating modelchain to forward analysisParamsDict.get('accuracy', 'low') into the analysis call (or drop this ini key if it’s not intended to be supported).
| accuracy: high |
| assert np.mean(results.Wm2Front[0]) == pytest.approx(899, rel = 0.005) # was 912 in v0.2.3 | ||
| assert np.mean(results.Wm2Back[0]) == pytest.approx(189, rel = 0.03) # was 182 in v0.2.2 | ||
| assert results.Pout[0] == demo2.compiledResults.Pout[0] == pytest.approx(369, abs= 1) | ||
| assert results.Mismatch[0] == pytest.approx(2.82, abs = .1) | ||
| assert results.Mismatch[0] == pytest.approx(2.82, abs = .15) |
There was a problem hiding this comment.
This test loosens the Mismatch tolerance, but it still won’t benefit from accuracy: high being added to ini_highAzimuth.ini because the modelchain path doesn’t currently propagate analysisParamsDict['accuracy'] into the analysis step. If the goal is to reduce variability, it would be better to plumb the ini’s accuracy setting through modelchain (so the test truly runs with high accuracy) rather than compensating by widening assertions.
| - name: Install Radiance v5.3 | ||
| run: | | ||
| wget https://github.com/LBNL-ETA/Radiance/releases/download/012cb178/Radiance_012cb178_Linux.zip -O radiance.zip | ||
| #wget https://github.com/LBNL-ETA/Radiance/releases/download/012cb178/Radiance_012cb178_Linux.zip -O radiance.zip | ||
| wget https://github.com/LBNL-ETA/Radiance/releases/download/rad6R0P2/Radiance_c1700d56_Linux.zip -O radiance.zip | ||
| unzip radiance.zip | ||
| tar -xvf radiance-5.3.012cb17835-Linux.tar.gz | ||
| #tar -xvf radiance-5.3.012cb17835-Linux.tar.gz | ||
| tar -xvf radiance-6.0.c1700d56cc-Linux.tar.gz |
There was a problem hiding this comment.
The step name says “Install Radiance v5.3”, but the commands now download/extract Radiance 6.0 (rad6R0P2 / radiance-6.0...). Please update the step name (and/or remove the commented-out v5.3 lines) to match what’s actually installed, so future debugging doesn’t get misleading signals.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Coverage Report for CI Build 26662835530Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Warning No base build found for commit Coverage: 74.781%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
[ ] Code changes have been evaluated for compatibility/integration with GUI[ ] New functions added to__init__.py[ ] manualapi.rst is up to date, along with other sphinx docs pages