-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Adopt dni_clear
variable name for clearsky DNI
#2274
Conversation
dni_clear
variable namedni_clear
variable name
ah I asked about this in the linked issue as I thought it might be, thanks @AdamRJensen |
dni_clear
variable namedni_clear
variable name
dni_clear
variable namedni_clear
variable name for clearsky DNI
Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These PRs will certainly need a whatsnew entry.
Has anyone audited iotools
to see if we can standardize any clearsky DNI names there too?
Co-Authored-By: Kevin Anderson <57452607+kandersolar@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly rST comments. PR looks good 🚀
Shall any .. deprecated
or .. versionchanged
directives be used? I'm +1 for the latter, I have the mild impression the former is for complete removals.
Docs at: https://pvlib-python--2274.org.readthedocs.build/en/2274/
Co-authored-by: Echedey Luis <80125792+echedey-ls@users.noreply.github.com>
Variable maps exist. pvlib-python/pvlib/iotools/psm3.py Lines 28 to 30 in 6af80da
pvlib-python/pvlib/iotools/solargis.py Lines 29 to 37 in 6af80da
I'm not 100% familiar with how these maps work but I guess instances of other variable names that are part of the map, e.g.: pvlib-python/pvlib/iotools/solcast.py Lines 355 to 363 in 6af80da
do not matter because they will be mapped to *_clear (?)
I think the variable name on the left is external and right is pvlib internal, but then it looks like Can someone advise whether additional revisions are needed? |
Thanks for checking. I think no action is needed in iotools then.
Yes, I think so. Right now the docstring doesn't have any indication that the name changed. I like how @echedey-ls did it here: https://github.com/echedey-ls/pvlib-python/blob/1eb44a5d5788b398ae3333ae5f96a04df6567dea/pvlib/solarposition.py#L1351-L1356 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Any comments from others (@echedey-ls?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM too!
After some thought, I think stating the version when the legacy param names will be removed (clarifying edit: in the docstring admonition) would be useful, but neither essential nor a blocker for merging this PR. We can do that in the future or after taking care of #2325. |
addedmodifiedUpdates entries indocs/sphinx/source/reference
for API changes.docs/sphinx/source/whatsnew
for 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.dni_clear
is already in the variables and symbols listAlso related: #1253, #2306