Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add N. Martin & J. M. Ruiz spectral response mismatch modifiers model #1658
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
Add N. Martin & J. M. Ruiz spectral response mismatch modifiers model #1658
Changes from 28 commits
a7d0070
60a63fd
93f1038
f606cd8
f79628a
a7b47b4
b9ec506
3a8630e
025c76e
f0ea498
8d4442e
399312b
f24aa92
c26b865
900ed98
eb1b245
149c557
2bf9d5b
be9554f
10ff9c2
6a2dbf0
3d5cceb
b16eaa6
f5352ac
d66f16e
db48a76
e7011ef
30c66b9
c04b70d
183320f
8730fcd
7ced74b
6385897
108def0
d75eea0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
If the model requires a specific air mass calculation, then perhaps it is better to have zenith angle as input.
This comment applies to a number of other existing pvlib functions as well, actually.
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.
Most functions in pvlib have less friendly input checking, which may be good or bad. You might just see what happens in each case without these checks and see whether the message python produces upon failure is comprehensible. In that case you may not need some of these custom messages.
Other than that I would prefer two nested if/else structures over the four combined conditions.
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 have tried what you say, but I don't get nice results - when both are provided, one of the inputs is ignored without telling so; and when neither of them are,
_params is not defined
is raised, which could be a good error message.In my humble opinion, being verbatim in the interface is not a problem, and is better than seeing a variable you did not write is not being defined. It will save time for the user, essentially.
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.
numpy and scipy use the None, None default pattern, where if both are None the function fails, so I think we're among good company.