-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Improve clarity of code in pvsyst_celltemp (names and doc) #651
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.
see comments on code lines.
Here are some additional thoughts about things I did not include in the PR:
I do have some new work on temperature models which will raise some additional interesting code integration questions, but that's still a few months away at best. |
I agree with these proposals. They make the low level functions more flexible and more explicit. The doc strings can point users to the presets in the module-level dictionary. The We'd need a deprecation cycle to make the same changes to |
Further changes anyone? |
Ok to merge if CI passes. |
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 should have checked the argument order for the method earlier, sorry I did not catch that. I can't tell why the test is failing, @wholmgren any ideas?
@cwhanse I think the failure was due to the mismatched arguments, but it looks ok now. I just noticed that The codecov test is failing for reasons unrelated to this PR and can be ignored. |
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.
one more minor fix
@adriesse thanks Anton! |
You're welcome! |
docs/sphinx/source/api.rst
for API changes.docs/sphinx/source/whatsnew
file for all changes.Brief description of the problem and proposed solution (if not already fully described in the issue linked to above):
I have tried to improve clarity of code in
pvsyst_celltemp
through changes to variables names and docstring, and also changed the name of one function parameter.