Improved error codes for irradproc, solar_resource_data

[mjprilliman]

-Fixes #973
-Add output errorcodes from irrad::check(), not just codes
-Add checks for solar_resource_data, including header information such as elev, lat, lon, tz
-Tested on pvwattsv8, feedback needed before propagating across irrad and solar_resource_data instances
@cpaulgilman review needed on language of error messages
@brtietz review needed on functionality

[mjprilliman]

pvwatts8_test.zip
phoenix_az_33.450495_-111.983688_psmv3_60_tmy_test.csv

[cpaulgilman]

@mjprilliman Thanks for tackling this. I'll look over and revise the text of the messages. I think the variable name "errorCode" is misleading. Not sure if it's worth the effort to change, but something like "error_message" would make it clearer that it's a text message and not a numeric code.

[cpaulgilman]

@mjprilliman I'm using PVWatts to test this in the SAM UI because it is the only model that calls irradproc's getErrorCode(). When I use this weather file with invalid time zone data, SAM crashes:

[mjprilliman]

@mjprilliman I'm using PVWatts to test this in the SAM UI because it is the only model that calls irradproc's getErrorCode(). When I use this weather file with invalid time zone data, SAM crashes:

Sorry Paul, there was an extra callout to code that was in the util::format call in pvwattsv8 that I had been using for troubleshooting. That meant it was trying to format a error code as a string, a string as a integer, and so on. The most recent push should fix that, and rename the errorCode references to errorMessage.

[cpaulgilman]

@mjprilliman This is working now in PVWatts, thanks for the fix.

I would like to be able to use formatted strings like this to include useful data in the error messages:

    if (latitudeDegrees < -90 || latitudeDegrees > 90 || longitudeDegrees < -180 || longitudeDegrees > 180 ||
        timezone < -15 || timezone > 15) {
        std::sprintf(errorMessage, "Invalid latitude (%f), longitude (%f), or time zone (%f)", latitudeDegrees, longitudeDegrees, timezone);
        return -2;
    }

But this doesn't work because errorMessage is a string, not the required char. I tried a few things but couldn't figure out how to do this. Is there an easy way to make this work? If not I'll work with the message text as it is.

[mjprilliman]

@mjprilliman This is working now in PVWatts, thanks for the fix.

I would like to be able to use formatted strings like this to include useful data in the error messages:

    if (latitudeDegrees < -90 || latitudeDegrees > 90 || longitudeDegrees < -180 || longitudeDegrees > 180 ||
        timezone < -15 || timezone > 15) {
        std::sprintf(errorMessage, "Invalid latitude (%f), longitude (%f), or time zone (%f)", latitudeDegrees, longitudeDegrees, timezone);
        return -2;
    }

But this doesn't work because errorMessage is a string, not the required char. I tried a few things but couldn't figure out how to do this. Is there an easy way to make this work? If not I'll work with the message text as it is.

Paul, I tested it out and something like this should work:

errorMessage = util::format("Invalid latitude (%lg), longitude (%lg), or time zone (%lg)", latitudeDegrees, longitudeDegrees, timezone);

Not a very elegant example but it reports out this:

[cpaulgilman]

@mjprilliman util::format is what I need -- thanks for the reminder~

[brtietz]

Are we looking for consistent behavior between cmods for this? For example, cmod_irradproc will throw an error for elevation < 0, but pvwattsv8 will just silently set it to zero and run just fine. Is that expected for optional variables?

[mjprilliman]

Are we looking for consistent behavior between cmods for this? For example, cmod_irradproc will throw an error for elevation < 0, but pvwattsv8 will just silently set it to zero and run just fine. Is that expected for optional variables?

For elevation specifically, I was moving away from error messages or text warnings for the changed value and leaning on the elevation output in the results spelling things out. This avoids breaking things like the CSP weather file reader that looks for warning messages whether they throw an error or not. Happy to figure out a workaround, and will take a look at cmod_irradproc.cpp

[brtietz]

Overall this is a big improvement and the error messages work well for irradiance, time, and albedo. The highlighted values from hour 2 all should generate warnings or errors, but PVWattsv8 runs just fine with them:

Do we want to fix these as a part of this PR, another issue, or are these unimportant?

[mjprilliman]

Overall this is a big improvement and the error messages work well for irradiance, time, and albedo. The highlighted values from hour 2 all should generate warnings or errors, but PVWattsv8 runs just fine with them:

Do we want to fix these as a part of this PR, another issue, or are these unimportant?

Tdew will throw an error if the heat transfer temperature model is chosen. The other place it is used is the POA decomposition, would need to check error handling there.
Wdir similarly is only referenced by the heat transfer temperature model and should have an error check there (0-360, non-nan)

Pressure and Tamb are treated as optional inputs to irrad same as elevation, so they get rewritten as 1013.25 mbar and 15 degC if the values are not valid (isnan check for each, > 800 mbar for pres). But Tamb should likely throw an error from the thermal model.

Wind speed should likely have a positive sanity check and an upper bound in the thermal models.

I vote for making a new issue for improving error messages and handling at the thermal model level.

[cpaulgilman]

I vote for making a new issue for improving error messages and handling at the thermal model level.

I agree with making a new issue for better error checking in the PV models for weather data elements that depend on options.

Note that the "Weather Data Elements" topic in Help attempts to document when different weather data elements are required for all performance models: https://samrepo.nrelcloud.org/help/weather_data_elements.html

[brtietz]

Approving the code here. Filed #1226 for additional improvements after the release.