Skip to content

Inappropriate reference for the function "dc_ohmic_losses" #1601

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
adriesse opened this issue Nov 25, 2022 · 4 comments · Fixed by #2229
Closed

Inappropriate reference for the function "dc_ohmic_losses" #1601

adriesse opened this issue Nov 25, 2022 · 4 comments · Fixed by #2229
Assignees
Milestone

Comments

@adriesse
Copy link
Member

Describe the bug
The function dc_ohmic_losses consists of one line:

return resistance * current * current

which is clearly an elementary electrical calculation.

The pvlib documentation provides a reference to the PVsyst documentation and organizes this function under the PVsyst banner in the documentation.

I think this is a bit strange, and could give the impression that this is something invented by/for PVsyst.

The same could be said about the function dc_ohms_from_percent.

Desired behavior
It would be preferable to have basic functions like this without such specific attributions/associations. A reference, if judged necessary, could be to a simple textbook. It could be useful to provide the information that PVsyst uses these elementary equations, which could be mentioned in the Notes section perhaps--ideally with information about other software too.

@adriesse adriesse changed the title In appropriate reference for the function "dc_ohmic_losses" Inappropriate reference for the function "dc_ohmic_losses" Nov 25, 2022
@cwhanse
Copy link
Member

cwhanse commented Dec 1, 2022

Listing of this function in the Pvsyst group of "PV System Models" documentation is meant to communicate that Pvsyst applies ohmic losses using this method (the "Array ohmic losses page" in the Help document). It wasn't intended to imply that Pvsyst originated the equation (the latter is usually indicated by the name of the function).

The function could certainly be listed in other groupings in the API documentation.

Maybe we should explain what we mean by "PV System Models" - in my mind these are collections of functions that together emulate some named model.

@adriesse
Copy link
Member Author

adriesse commented Dec 1, 2022

Yes, and perhaps we should try to distinguish between models and software. For me, PVsyst is a software package which implements some generic/basic models/equations, some tweaked standard models, and some original models.

@wholmgren
Copy link
Member

Perhaps we can just change these lines (as the title of the issue suggests) and leave the API documentation questions for other day since that's already in flux in other places:

pvlib-python/pvlib/pvsystem.py

Lines 3317 to 3321 in 8e0b724

References
----------
.. [1] PVsyst 7 Help. "Array ohmic wiring loss".
https://www.pvsyst.com/help/ohmic_loss.htm
"""

pvlib-python/pvlib/pvsystem.py

Lines 3352 to 3356 in 8e0b724

References
----------
.. [1] PVsyst 7 Help. "Array ohmic wiring loss".
https://www.pvsyst.com/help/ohmic_loss.htm
"""

@cwhanse
Copy link
Member

cwhanse commented Dec 1, 2022

Thumbs up also to only change the references in the docstrings, but I would like to at least retain a comment that connects these functions with the way Pvsyst does things. If for no other reason, for future work on something like ModelChain.with_pvsyst

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants