Gen may be longer than any merchant plant price array.

[brtietz]

Pull Request Template

Description

Check the length of gen when setting up nsteps for price arrays. A subhourly gen may be the longest array.

Fixes NatLabRockies/SAM#2161

Corresponding branches and PRs:

Develop on other branches

Unit Test Impact:

No changes expected

Checklist

• requires help revision and I added that label
• adds, removes, modifies, or deletes variables in existing compute modules
• adds a new compute module
• changes defaults
• I've tagged this PR to a milestone

Reminders- this section can be deleted

[Checking for PySAM Incompatible API Changes]
(https://github.com/NREL/SAM/wiki/PySAM-Incompatible-API-Changes-&-Regenerating-PySAM-Files).

[When do the PySAM files need to be regenerated?]
(https://github.com/NREL/SAM/wiki/PySAM-Incompatible-API-Changes-&-Regenerating-PySAM-Files#when-do-the-pysam-files-need-to-be-regenerated-via-export_config)

[mjprilliman]

Can't upgrade Gtest yet but the failing test is CmodMerchantPlantTest.CustomGenerationBattery_LCOS

[brtietz]

Can't upgrade Gtest yet but the failing test is CmodMerchantPlantTest.CustomGenerationBattery_LCOS

It looks like that test is failing because it had one minute input data, therefore we'd expect the results to change with this PR. Does that make sense to you?

However, when trying to recreate the test data, I get:

exec fail(battery): The requested number of timesteps must be a multiple of 8760. Instead requested timesteps is 525600.
Simulation battery failed :exec fail(battery): The requested number of timesteps must be a multiple of 8760. Instead > requested timesteps is 525600.

525600 is the expected number for one minute data, so that shouldn't throw. I expect the error handling over on #1384 is better for this, so I might redirect this PR and merge that branch into this one.

[mjprilliman]

Can't upgrade Gtest yet but the failing test is CmodMerchantPlantTest.CustomGenerationBattery_LCOS

It looks like that test is failing because it had one minute input data, therefore we'd expect the results to change with this PR. Does that make sense to you?

However, when trying to recreate the test data, I get:

exec fail(battery): The requested number of timesteps must be a multiple of 8760. Instead requested timesteps is 525600.
Simulation battery failed :exec fail(battery): The requested number of timesteps must be a multiple of 8760. Instead > requested timesteps is 525600.

525600 is the expected number for one minute data, so that shouldn't throw. I expect the error handling over on #1384 is better for this, so I might redirect this PR and merge that branch into this one.

https://github.com/NatLabRockies/ssc/blob/develop/ssc/common.cpp#L976C9-L977C87 Seems like it's an issue with forcast_price_signal::setup() in common.cpp. If line 976 is triggered and resets step_per_hour, I don't see how 977 can not be triggered unless nyears is 1.

[brtietz]

Can't upgrade Gtest yet but the failing test is CmodMerchantPlantTest.CustomGenerationBattery_LCOS

It looks like that test is failing because it had one minute input data, therefore we'd expect the results to change with this PR. Does that make sense to you?
However, when trying to recreate the test data, I get:

exec fail(battery): The requested number of timesteps must be a multiple of 8760. Instead requested timesteps is 525600.
Simulation battery failed :exec fail(battery): The requested number of timesteps must be a multiple of 8760. Instead > requested timesteps is 525600.

525600 is the expected number for one minute data, so that shouldn't throw. I expect the error handling over on #1384 is better for this, so I might redirect this PR and merge that branch into this one.

https://github.com/NatLabRockies/ssc/blob/develop/ssc/common.cpp#L976C9-L977C87 Seems like it's an issue with forcast_price_signal::setup() in common.cpp. If line 976 is triggered and resets step_per_hour, I don't see how 977 can not be triggered unless nyears is 1.

Agreed. I think when I wrote that code ~4 years ago I copied 976 over unnecessarily, so I removed it just now. I think it's safe to assume the upstream battery code as a good handle on steps per hour and it shouldn't change here.

I wasn't awake enough to notice that this was a battery bug and not a merchant plant bug (even though it's basically the same routine interpreting these arrays). So I changed the PR target even though it wasn't strictly necessary since that seems hard to undo for not a lot of benefit.

[coveralls]

Coverage Report for CI Build 24407492393

Coverage increased (+0.003%) to 56.278%

Details

• Coverage increased (+0.003%) from the base build.
• Patch coverage: No coverable lines changed in this PR.
• 313 coverage regressions across 6 files.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

313 previously-covered lines in 6 files lost coverage.

File	Lines Losing Coverage	Coverage
ssc/ssc/cmod_merchantplant_eqns.cpp	90	55.2%
ssc/shared/lib_pv_io_manager.cpp	88	83.95%
ssc/ssc/common.cpp	88	76.07%
ssc/ssc/cmod_pvwattsv8.cpp	36	82.83%
ssc/shared/lib_battery.cpp	7	89.87%
ssc/shared/lib_battery_powerflow.cpp	4	95.94%

---

Coverage Stats

Relevant Lines:	121163
Covered Lines:	68188
Line Coverage:	56.28%
Coverage Strength:	3452574.12 hits per line

---

💛 - Coveralls

[cpaulgilman]

Looks good. Thanks for the fix!