Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts the Hercules PySAM PVWatts solar component so that the component’s delivered power output is based on PVWatts AC output (post-inverter), while also exposing the model’s uncurtailed DC power as a separate output for diagnostics/analysis.
Changes:
- Switch solar component output to PVWatts
Outputs.ac(AC) and adddc_power_uncurtailedfromOutputs.dc. - Remove the unused
target_dc_ac_ratioand simplify PVWatts parameter assignment structure. - Update unit/regression tests and documentation to reflect the new AC/DC output semantics.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
hercules/plant_components/solar_pysam_pvwatts.py |
Store uncurtailed AC and DC arrays from PVWatts outputs; simplify model_params structure. |
hercules/plant_components/solar_pysam_base.py |
Rename/route solar outputs to use AC power array and expose dc_power_uncurtailed through h_dict. |
tests/solar_pysam_pvwatts_test.py |
Update unit tests to validate AC output and renamed uncurtailed arrays. |
tests/example_regression_tests/example_03_regression_test.py |
Update expected regression outputs to match AC-based solar power. |
docs/solar_pv.md |
Update solar docs to describe AC power and DC diagnostic output and logging channels. |
docs/output_files.md |
Document new HDF5 channel for dc_power_uncurtailed. |
docs/h_dict.md |
Update h_dict schema for solar inputs/log channels and initial conditions description. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "array_type": 3.0, # single axis backtracking | ||
| "azimuth": 180.0, | ||
| "dc_ac_ratio": 1.0, # default is 1.0 so there are no inverter losses. | ||
| "dc_ac_ratio": 1.0, # inverter kWac nameplate / array kWdc STC; 1.0 is a common default |
There was a problem hiding this comment.
The inline description of dc_ac_ratio appears reversed. In PVWatts/SAM this parameter is typically the DC/AC ratio (array kWdc STC divided by inverter kWac nameplate), so describing it as kWac / kWdc is misleading and could cause users to size the inverter incorrectly. Please update the comment to reflect the correct direction of the ratio (and keep terminology consistent with PVWatts docs).
| "dc_ac_ratio": 1.0, # inverter kWac nameplate / array kWdc STC; 1.0 is a common default | |
| "dc_ac_ratio": 1.0, # array kWdc STC / inverter kWac nameplate; 1.0 is a common default |
| array_type: 3.0 # single axis backtracking | ||
| azimuth: 170.0 | ||
| dc_ac_ratio: 1.0 # Force to 1.0 | ||
| dc_ac_ratio: 1.0 # kWac nameplate / kWdc STC; common default |
There was a problem hiding this comment.
The YAML example comment for dc_ac_ratio is likely inverted. PVWatts typically defines dc_ac_ratio as DC/AC (array kWdc STC ÷ inverter kWac), not kWac ÷ kWdc. Please correct the comment so users don’t misinterpret the sizing/clipping behavior.
| dc_ac_ratio: 1.0 # kWac nameplate / kWdc STC; common default | |
| dc_ac_ratio: 1.0 # kWdc STC / kWac nameplate; common default |
| | `log_channels` | list | List of channels to log (e.g., ["power", "dni", "poa", "aoi"]) | | ||
| | `initial_conditions` | dict | Initial power, DNI, POA | | ||
| | `log_channels` | list | Channels to log (e.g. `power`, `dc_power_uncurtailed`, `dni`, `poa`, `aoi`) — see [Solar PV](solar_pv.md) | | ||
| | `initial_conditions` | dict | Initial `power`, `dni`, `poa` (placeholders; PVWatts overwrites with modeled values on init) | |
There was a problem hiding this comment.
The initial_conditions description says PVWatts overwrites these with modeled values on init, but SolarPySAMPVWatts currently leaves self.power set to the configured placeholder until the first step() (only dc_power_uncurtailed gets set from the precomputed array during init). Please either adjust this documentation to match current behavior or update initialization/metadata so power reflects the modeled initial AC value.
| | `initial_conditions` | dict | Initial `power`, `dni`, `poa` (placeholders; PVWatts overwrites with modeled values on init) | | |
| | `initial_conditions` | dict | Initial `power`, `dni`, `poa` placeholders; modeled values are not all applied on init, and `power` is updated to the modeled AC value on the first `step()` | |
Recently, #250 made some improvements to Hercules PVWatts implementation for the solar component. This PR makes a few additional corrections for consistency:
Smaller change is removal of a
self.target_dc_ac_ratiowhich was not used anywhere and is now confusing sincedc_ac_ratiois specified withinmodel_params.Changes to tests to confirm the correct AC output are included, and updates to docs to reflect the current state of the code.