pvlib-python icon indicating copy to clipboard operation
pvlib-python copied to clipboard

[WIP] Adopt `dni_clear` variable name

Open RDaxini opened this issue 1 year ago • 2 comments

Not ready for review

  • [X] Relates to #2272
  • [X] I am familiar with the contributing guidelines
  • [ ] Tests ~added~ modified
  • [ ] ~Updates entries in docs/sphinx/source/reference for API changes.~
  • [ ] Adds description and name entries in the appropriate "what's new" file in 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`).
  • [X] New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • [ ] Pull request is nearly complete and ready for detailed review.
  • [ ] Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

dni_clear is already in the variables and symbols list Also related: #1253

RDaxini avatar Oct 21 '24 21:10 RDaxini

In both this PR and in #2273, you should use the decorate added in #2237.

The reason is that simply changing the parameter name would be a breaking change.

AdamRJensen avatar Oct 22 '24 02:10 AdamRJensen

In both this PR and in #2273, you should use the decorate added in #2237.

The reason is that simply changing the parameter name would be a breaking change.

ah I asked about this in the linked issue as I thought it might be, thanks @AdamRJensen

RDaxini avatar Oct 23 '24 15:10 RDaxini

Has anyone audited iotools to see if we can standardize any clearsky DNI names there too?

Variable maps exist. psm3.py, solargis.py, sodapro.py, solcast.py, and solaranywhere.py map to *_clear, e.g: https://github.com/pvlib/pvlib-python/blob/6af80da35a7c96059c534ee38be9123bcfc7f50f/pvlib/iotools/psm3.py#L28-L30 https://github.com/pvlib/pvlib-python/blob/6af80da35a7c96059c534ee38be9123bcfc7f50f/pvlib/iotools/solargis.py#L29-L37

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.: https://github.com/pvlib/pvlib-python/blob/6af80da35a7c96059c534ee38be9123bcfc7f50f/pvlib/iotools/solcast.py#L355-L363 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 *_clear was already there before it was used in these functions in pvlib...? I might have misunderstood something here.

Can someone advise whether additional revisions are needed?

RDaxini avatar Dec 05 '24 17:12 RDaxini

Thanks for checking. I think no action is needed in iotools then.

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.

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

kandersolar avatar Dec 06 '24 15:12 kandersolar

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.

echedey-ls avatar Dec 07 '24 22:12 echedey-ls