Allow delta_t to be array in SPA functions
#2190
Conversation
There was a problem hiding this comment.
Ready for review. The coverage complaints are the usual numba issue; they can be ignored.
A brief explanation: the complexity here is to accommodate numba. With the numpy calculations, duck typing means both scalar and array delta_t are fine. But with numba, we have to pick a single dtype. This PR makes delta_t always an array (forcing scalars to arrays if needed) so that the numba code can deal with arrays only.
Technically this change will increase memory use, but my informal benchmarking found no noticeable slowdown. Considering what a computational beast the SPA is, I'm not surprised that one extra array doesn't make any measurable difference.
| def solar_position_loop(unixtime, loc_args, out): | ||
| @jcompile('void(float64[:], float64[:], float64[:], float64[:,:])', | ||
| nopython=True, nogil=True) | ||
| def solar_position_loop(unixtime, delta_t, loc_args, out): |
There was a problem hiding this comment.
Whereas delta_t used to be a scalar that could live in loc_args, now it is a time-varying value (same length as unixtime) and thus requires a separate array input. So a new parameter is needed, and loc_args shrinks by one.
| @pytest.mark.parametrize('method', [ | ||
| 'nrel_numpy', | ||
| 'ephemeris', | ||
| pytest.param('pyephem', marks=requires_ephem), | ||
| pytest.param('nrel_numba', marks=requires_numba), | ||
| pytest.param('nrel_c', marks=requires_spa_c), | ||
| ]) | ||
| @pytest.mark.parametrize('tz', [None, 'utc', 'US/Eastern']) |
There was a problem hiding this comment.
This order change was so that all the numba tests run in sequence; the previous ordering interleaved numpy & numba calls, causing unnecessary reloading of pvlib.spa.
| sp_us = solarposition.get_solarposition(index_us, 40, -80, method=method) | ||
| with warnings.catch_warnings(): | ||
| # don't warn on method reload | ||
| warnings.simplefilter("ignore") |
There was a problem hiding this comment.
This change is not directly related; just a tweak to suppress nuisance reloading warnings
|
A better test instead of [65, 65] would be to have an array input with different values, e.g., [65, 66]. |
Co-authored-by: Adam R. Jensen <[email protected]>
|
@kandersolar Thanks for the test update. I'll merge this tomorrow before the next release unless I hear any objections. |
solarposition.spa_pythondoes not accept arrays fordelta_tparameter #2189docs/sphinx/source/referencefor API changes.docs/sphinx/source/whatsnewfor all changes. Includes link to the GitHub Issue with:issue:`num`or this Pull Request with:pull:`num`. Includes contributor name and/or GitHub username (link with:ghuser:`user`).remote-data) and Milestone are assigned to the Pull Request and linked Issue.See #2189. Additional tests are needed before this is ready for detailed review.