PV models speed up#1212
Conversation
mjprilliman
left a comment
There was a problem hiding this comment.
This looks great! I want to double check seasonal tilt behavior, so I may need to do a bit more debugging.
mjprilliman
left a comment
There was a problem hiding this comment.
Looks good to me. Just one merge conflict from recent merges to address
|
This looks good to me! I think it makes sense to update the pvwattsv8 expected test values to the new values as well. I like scaling the error tolerance by annual energy, but I think I'd like to make the error tolerance itself smaller if we're keeping that approach. Requesting an additional review from @sjanzou , particularly on the hash table, just because this is a big change for pvwatts and pvsam. Super excited for these results @dguittet !!! Nice job!!! |
|
Need tests for Alaska and South Africa |
sjanzou
left a comment
There was a problem hiding this comment.
Looks good overall, just a few comments and questions on some of the implementation values and sensitivities.
Thanks for tackling this!
| ascension_and_declination[0], ascension_and_declination[1]}; | ||
|
|
||
| if (use_table) | ||
| spa_table[spa_key_inputs] = spa_outputs; |
There was a problem hiding this comment.
Have you run with use_table = false to verify previous results?
There was a problem hiding this comment.
Yes. I'm planning to remove this code once we're done examining sensitivities-- thoughts on deleting this?
| spa_table_key(double j, double dt, double p, double t, double a, double d): | ||
| jd(j), delta_t(dt), ascension(a), declination(d) | ||
| { | ||
| int pressure_bucket = 10; |
There was a problem hiding this comment.
Do you have a sensitivity plot based on pressure and temp bucket size?
There was a problem hiding this comment.
Sure, the sensitivity is super low to pressure and temperature:
| pressure bucket | temp bucket | time (ms) | result change |
|---|---|---|---|
| 10 | 5 | 3250 | na |
| 2 | 5 | 3443 | na |
| 2 | 2 | 3490 | na |
| 10 | 2 | 3411 | na |
| 1 | 1 | 3630 | na |
| 20 | 10 | 3238 | na |
Ran Pvwattsv8 tests for Alaska and tip of South America and the changes in this PR (both self shading and solarpos), and checked changes in annual energy and monthly energy. The max change over those 26 values was < 0.02%. |
sjanzou
left a comment
There was a problem hiding this comment.
Thanks for tackling this!
Speed up pvsamv1, pvwattsv7 and pvwattsv8 by speeding up irradproc and sssky_diffuse_table.
irradproc:
calculate_spathat reduces the number of times thatcalculate_spais called per timestepsolarpos_spain a vector so that the values are not recalculated each yearsssky_diffuse_table:
sprintftakes a long timesssky_diffuse_table::computepvwattsv8 and pvwattsv7:
pvsamv1:
tests:
Speed changes
Before is the original code, "Spa Hash" is what is implemented in this PR