Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Solar Resource Data does not throw errors for bad fields #973

Open
brtietz opened this issue Jan 23, 2023 · 3 comments · May be fixed by #1219
Open

Solar Resource Data does not throw errors for bad fields #973

brtietz opened this issue Jan 23, 2023 · 3 comments · May be fixed by #1219
Assignees

Comments

@brtietz
Copy link
Collaborator

brtietz commented Jan 23, 2023

Describe the bug
A PySAM user was attempting to use Solar Resource Data to drive a PV simulation. The simulation provided worked fine with a weather file, but the solar resource data had a bad key "Elevation" instead of "elev". No error was thrown, but DC power was not calculated.

To Reproduce
TODO: upload example workbook

Expected behavior
The code should throw errors for this case.

Desktop (please complete the following information):

  • Version PySAM 3.0.2, 4.0.0
@mjprilliman
Copy link
Collaborator

I wasn't able to duplicate this behavior when running example scripts. The solar_resource_data did not have an 'elev' key but the simulation still ran and generated dc power results and gen results. The same happened when I properly added an 'elev' key. I think there's enough uncertainty on what should be reported and at what verbosity level to maybe punt this to the release? @cpaulgilman @dguittet what should be printed when optional parameters aren't added to the resource dictionaries?

@cpaulgilman
Copy link
Collaborator

I wasn't able to duplicate this behavior when running example scripts. The solar_resource_data did not have an 'elev' key but the simulation still ran and generated dc power results and gen results. The same happened when I properly added an 'elev' key. I think there's enough uncertainty on what should be reported and at what verbosity level to maybe punt this to the release? @cpaulgilman @dguittet what should be printed when optional parameters aren't added to the resource dictionaries?

For weather files, the wfcheck module is available to check solar resource data, but there is not a similar module for checking data provided as a table. I agree with moving this to the Fall 2023 release given the level of effort to fix it. Some notes:

  • The solar_resource_data option is used by different solar modules (PV, CSP, etc.), so the implementation for checking the data should be available to all solar modules that need it.

  • The wfcheck module only checks the weather data itself. It should probably also check required header data (latitude, longitude, time zone, and elevation).

  • The solar models (pvsamv1, pvwattsv8, CSP, water heating, etc.) require the elevation field for sun position, angle of incidence, temperature and other calculations.

@cpaulgilman
Copy link
Collaborator

When you use the weather data option with no weather file, the only checks are done by the performance model (pvwattsv8, pvsamv1, etc.), which calls irrad::check() in irradproc to provide feedback about the weather data. For example, for data with an invalid DNI value, pvwattsv8 reports the cryptic “Error: exec fail(pvwattsv8): Failed to process irradiation on surface (code: -105) [year:2004 month:1 day:1 hour:12 minute:30]. time -1.000000”.

For header data, the irrad::check() module has a check for lat/lon but not elevation or time zone.

I think the solution is:

  1. Report descriptions of errors instead of error codes.
  2. Make checking of weather data consistent for weather file input and weather data input.

@cpaulgilman cpaulgilman self-assigned this Jul 30, 2024
@cpaulgilman cpaulgilman removed their assignment Oct 9, 2024
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 a pull request may close this issue.

3 participants