-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add optional arguments temperature_ref
and irradiance_ref
to pvsystem.sapm
#2434
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
Conversation
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.
I'm unsure about the reference names here, no strong opinion.
Anyway, as always, good work @RDaxini.
pvlib/pvsystem.py
Outdated
@@ -2195,7 +2195,8 @@ def _parse_raw_sam_df(csvdata): | |||
return df | |||
|
|||
|
|||
def sapm(effective_irradiance, temp_cell, module): | |||
def sapm(effective_irradiance, temp_cell, module, reference_temperature=25, |
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.
def sapm(effective_irradiance, temp_cell, module, reference_temperature=25, | |
def sapm(effective_irradiance, temp_cell, module, *, reference_temperature=25, |
Consider using kwarg-only parameters to enforce readability of code. For future issues debugging, ...
Completely optional suggestion, it's not a common trend in pvlib.
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.
I'm not familiar with what this is doing actually... could you elaborate or forward a link for me to review?
You noted it's optional / uncommon in pvlib currently, so I'll just let others comment whether to go ahead with this as I have no idea 😅
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.
Quick lookup on the internet and you will have a ton of information more extensive, but here is a short example, where all statements are expected to print Hola Dax
. The star means the following arguments are only allowed to be passed in as keyword-only (parameter_name=value):
def hola_dax(greet, name):
print(f"{greet} {name}")
# allowed uses
hola_dax("Hola", "Dax")
hola_dax("Hola", name="Dax")
hola_dax(greet="Hola", name="Dax")
hola_dax(name="Dax", greet="Hola")
# however
def hola_dax(greet, *, name):
print(f"{greet} {name}")
# only allows using the forms:
hola_dax("Hola", name="Dax")
hola_dax(greet="Hola", name="Dax")
# this raises an error:
hola_dax("Hola", "Dax") # TypeError: hola_dax() takes 1 positional argument but 2 were given
# and the form
def hola_dax(*, greet, name):
print(f"{greet} {name}")
# only allows:
hola_dax(greet="Hola", name="Dax")
hola_dax(name="Dax", greet="Hola")
This way, you can always enforce people to make readable code.
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.
Right ok got it! Seems like a good suggestion to me. Thanks @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.
https://pvlib-python--2434.org.readthedocs.build/en/2434/reference/generated/pvlib.pvsystem.sapm.html#pvlib.pvsystem.sapm
Should *
show here in the function, and if so is there any need to document that below in the parameter descriptions...?
reference_temperature
and reference_irradiance
to pvsystem.sapm
temperature_ref
and irradiance_ref
to pvsystem.sapm
Co-Authored-By: Echedey Luis <[email protected]>
This reverts commit e8a7b10.
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 :)
Minor rendering issue in the whatsnew down below.
Co-authored-by: Echedey Luis <[email protected]>
Thanks @RDaxini! |
pvsystem.sapm
#2432Tests addedUpdates 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.Related: #825