Skip to content

Consistent names for reference kwargs #825

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cwhanse opened this issue Nov 25, 2019 · 8 comments
Open

Consistent names for reference kwargs #825

cwhanse opened this issue Nov 25, 2019 · 8 comments

Comments

@cwhanse
Copy link
Member

cwhanse commented Nov 25, 2019

Is your feature request related to a problem? Please describe.
Both irrad_refand reference_irradiance are used for the reference irradiance kwarg for effective irradiance and DC models.

Describe the solution you'd like
Suggest it is worth changing to reference_irradiance throughout, and to replace temp_ref by reference_temperature.

@adriesse
Copy link
Member

adriesse commented Dec 6, 2019

Consistency would be great. I'm not a fan of really long names, but you already knew that.

@adriesse
Copy link
Member

adriesse commented Dec 6, 2019

I just came across T0 and E0 in pvsyst_parameter_estimation...

@wholmgren wholmgren modified the milestones: 0.8.0, 0.9.0 Aug 28, 2020
@cwhanse
Copy link
Member Author

cwhanse commented Sep 4, 2020

@wholmgren what is your view of the benefit of this proposed change?

@wholmgren
Copy link
Member

@cwhanse I'm +1 on changing them. I like your suggestions, but here are some quick searches for consistency...

I see pvterms has a different set of suggestions, irradiance_ref and temperature_ref (I'm not committing to using pvterms but I am committing to checking it). I see reference_irradiance is a keyword argument in PVSystem.sapm_effective_irradiance though it doesn't do anything. I don't see *reference* elsewhere in the library. A handful of parameters and functions use *_ref.

To @adriesse's point, I'm a fan of really long names, but you already knew that.

@wholmgren
Copy link
Member

@cwhanse there's also the ivtools.utility.constants dict

constants = {'E0': 1000.0, 'T0': 25.0, 'k': 1.38066e-23, 'q': 1.60218e-19}

@echedey-ls
Copy link
Contributor

It seems like we already have the tools to fix this issue (the variable naming table, and the deprecated kwarg decorator) and it would be a great benefit.

I think this issue deserves a vote on where the reference indicator should be in a parameter name (independently of the last trends):

  • 👍 : *_reference
  • 🎉 : reference_*
  • 🚀 : *_ref
  • 👀 : ref_*

@cwhanse
Copy link
Member Author

cwhanse commented Dec 22, 2024

Either 👍 or 🚀 are OK with me. I prefer placing the adjective 'reference' after the noun 'irradiance'

@adriesse
Copy link
Member

Let's allow for exceptions to the rule, if something sounds awkward.

RDaxini added a commit to RDaxini/pvlib-python that referenced this issue Apr 11, 2025
kandersolar pushed a commit that referenced this issue Apr 21, 2025
…ystem.sapm` (#2434)

* add ref irr/t as optional args, rename

* add terms to docs

* linter

* Update v0.12.1.rst

* move whatsnew entry from docs to enhancements

* multiple instances of C -> °C

* name change (#825)

* Update v0.12.1.rst

* missing `

* remove unused package

* change to kwarg only parameters

Co-Authored-By: Echedey Luis <[email protected]>

* Revert "Update v0.12.1.rst"

This reverts commit e8a7b10.

* update names, remove duplicate entry

* remove ivtools line. mistakenly added during merge branch?

* Update docs/sphinx/source/whatsnew/v0.12.1.rst

Co-authored-by: Echedey Luis <[email protected]>

---------

Co-authored-by: Echedey Luis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants