Repository navigation
Conversation
RDaxini
left a comment
RDaxini
left a comment
There was a problem hiding this comment.
Thanks for your contribution.
Can you confirm you read the issue? The issue never discussed adding tests to check the existing deprecation behavior and I see no note from yourself in the whatsnew file about these tests. Were these automatically generated by an AI agent? It is pvlib policy to vet all AI-generated content, which I understand you have confirmed you did in the checklist, but in this case I am unsure then how or why these tests appeared.
| (:issue:`1715`, :pull:`2883`) | ||
| * Note deprecation of ``king`` model in :py:func:`~pvlib.irradiance.get_total_irradiance` | ||
| and :py:func:`~pvlib.irradiance.get_sky_diffuse` docstrings. | ||
| (:issue:`2897`) |
There was a problem hiding this comment.
| (:issue:`2897`) | |
| (:issue:`2897`, :pull:`2904`) |
| def test_get_sky_diffuse_king_deprecated(irrad_data, ephem_data): | ||
| with warnings.catch_warnings(): | ||
| warnings.simplefilter('always') | ||
| with pytest.warns(pvlibDeprecationWarning, match='king'): | ||
| irradiance.get_sky_diffuse( | ||
| 40, 180, ephem_data['apparent_zenith'], | ||
| ephem_data['azimuth'], irrad_data['dni'], | ||
| irrad_data['ghi'], irrad_data['dhi'], model='king', | ||
| ) | ||
|
|
||
|
|
||
| def test_get_total_irradiance_king_deprecated(irrad_data, ephem_data): | ||
| with warnings.catch_warnings(): | ||
| warnings.simplefilter('always') | ||
| with pytest.warns(pvlibDeprecationWarning, match='king'): | ||
| irradiance.get_total_irradiance( | ||
| 40, 180, ephem_data['apparent_zenith'], | ||
| ephem_data['azimuth'], irrad_data['dni'], | ||
| irrad_data['ghi'], irrad_data['dhi'], model='king', | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
The issue #2897 never discussed tests. The issue discussed a docs change only. These tests check behavior that already exists and are not related to the issue so I don't think they should be included in this PR.
|
Thanks for the review. You're right, #2897 only asked for the docstring change. The tests were added by the AI assistant I used. When I vetted them I checked that they pass, but not whether they belonged in this PR, and they don't. I've removed them, applied your whatsnew suggestion and merged main to fix the conflict. |
RDaxini
left a comment
RDaxini
left a comment
There was a problem hiding this comment.
I advise reading the issue before throwing an AI agent at it in future as it would save everyone's time and resources (including yours, and the AI agents'). Given the simplicity of this PR, it's passable, but more complex PRs with the same problem are likely to be closed before review—just FYI.
Thanks for addressing this. LGTM now but we'll wait for another maintainer.
2 participants
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`).Note that the
kingdiffuse irradiance model is deprecated in the docstrings ofget_total_irradianceandget_sky_diffuse.