Skip to content

Conversation

@berg-michael
Copy link
Collaborator

@berg-michael berg-michael commented Jul 3, 2025

Description

The PR enables the builtin snow depth array to which the nohrsc_snow_data branch of SAM writes to. It allows the user to choose which source of snow data they wish to use: weather file provided or builtin array.

If the snow data is provided via the weather file, as before, there was an implicit guarantee that the snow depth data was of the same period as the weather file. This guarantee is lost when using a user-input array.

Thus, this PR also includes code the verify that the snow depth array can be aligned with the weather file data. Periods of 5 minutes and 10 minutes, for example are compatible, whereas periods of 5 and 12 minutes are not because there are timesteps that will not line up. In the case of incompatible periods, the simulation fails. In the case of matching periods, the simulation succeeds without any massaging of the snow depth data. In the case of non-matching but compatible periods, the snow data will be up- or down-sampled to align with the period of the weather file data.

To review, build against the nohrsc_snow_data branch of sam. Verify that snow data input into the array is used and has behavior that aligns with entering snow data into the weather file via the snow macro.

Corresponding branches and PRs:

branch: sam/nohrsc_snow_data

NREL/SAM#2089
https://github.com/NREL/SAM-private/pull/128

Unit Test Impact:

New tests written to exercise the snow array. They check:

  • using the snow_array compared to weather file data produces the same results
  • Upsampling the snow_array to match with a weather file with shorter period
  • Downsampling the snow_array to

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 (the corresponding SAM PR adds them)
  • I've tagged this PR to a milestone

@coveralls
Copy link

coveralls commented Jul 3, 2025

Pull Request Test Coverage Report for Build 16379348263

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 293 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.009%) to 56.658%

Files with Coverage Reduction New Missed Lines %
ssc/ssc/cmod_pvsamv1.cpp 138 87.4%
ssc/shared/lib_pv_io_manager.cpp 155 83.92%
Totals Coverage Status
Change from base Build 16200885485: 0.009%
Covered Lines: 66652
Relevant Lines: 117640

💛 - Coveralls

…ion with user entered snow depth data. Need to cycle back to beginning of the snow depth data for each year
@berg-michael
Copy link
Collaborator Author

Recent commit fixes the issue where PV + Financial Model simulations led to errors.

@berg-michael berg-michael requested a review from cpaulgilman July 4, 2025 01:14
@cpaulgilman cpaulgilman added this to the SAM Fall 2025 Release milestone Jul 21, 2025
Copy link
Collaborator

@sjanzou sjanzou left a comment

Choose a reason for hiding this comment

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

Working and approved if SAM 2089 GitHub actions are working

@cpaulgilman cpaulgilman merged commit bc66bc2 into develop Jul 23, 2025
10 checks passed
@cpaulgilman cpaulgilman deleted the nohrsc_snow_data branch July 23, 2025 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pv photovoltaic, pvsam, pvwatts requires SAM Help revision

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants