Skip to content

Docstring for pvsystem.calcparams_xxx are inconsistent with function code #716

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

Closed
cwhanse opened this issue May 9, 2019 · 5 comments
Closed

Comments

@cwhanse
Copy link
Member

cwhanse commented May 9, 2019

Docstrings for pvsystem.calcparams_desoto, pvsystem.calcparams_cec and pvsystem.calcparams_pvsyst list outputs as photocurrent etc. but code uses different terms, e.g., IL. Using the long variable names inside the functions would clarify the code.

Might want to consider returning dict rather than tuple.

@wholmgren
Copy link
Member

Ok with me to change the code to use long names. But I don't think the long/short inconsistency between the docstring/API and internal variable names is necessarily a problem. Our Contributing docs say:

pvlib python uses a mix of full and abbreviated variable names. See Variables and Symbols. We could be better about consistency. Prefer full names for new contributions. This is especially important for the API. Abbreviations can be used within a function to improve the readability of formulae.

@cwhanse
Copy link
Member Author

cwhanse commented May 9, 2019

Maybe adding a comment line with the return statement will close the bit of gap between the docstring and the variables used in the code. I think it is helpful to use the common symbols as variable names. I don't think including the symbols in the docstring would improve matters; seems like step back from the Contributing goal of full names.

@cwhanse cwhanse added this to the 0.6.4 milestone Jun 11, 2019
@cwhanse cwhanse modified the milestones: 0.6.4, 0.7.0 Jun 25, 2019
@wholmgren wholmgren modified the milestones: 0.7.0, 0.7.1 Nov 26, 2019
@wholmgren wholmgren modified the milestones: 0.7.1, 0.7.2 Jan 10, 2020
@CameronTStark CameronTStark modified the milestones: 0.7.2, 0.8.0 Mar 1, 2020
@wholmgren wholmgren modified the milestones: 0.8.0, Someday Aug 28, 2020
@pratham15541
Copy link

Can i do like
Updating the docstrings for the mentioned functions to match the variable names used in the code.
Modifying the functions to use long variable names for clarity.
Consider changing the return type of the functions from a tuple to a dictionary to improve readability.

@cwhanse
Copy link
Member Author

cwhanse commented Feb 18, 2025

The agreement here was to add some comments, e.g. for calcparams_desoto at this line, explaining how the internal variables map to the named outputs.

Changing the output type would require a much longer discussion that we had in this issue.

@cwhanse
Copy link
Member Author

cwhanse commented Mar 31, 2025

Closed by #2405

@cwhanse cwhanse closed this as completed Mar 31, 2025
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