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.
coefficient estimation method following DeSoto(2006) #784
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
coefficient estimation method following DeSoto(2006) #784
Changes from 36 commits
c5287ea
5a39fea
7b24201
c16f1d0
e7ef571
cdb2a73
0cfe13f
0c27829
d8653cd
bbb4580
6b8fadf
121cb06
47e1be4
c1b62c4
4e62d1a
747ed48
9ea2ed8
bd39dbd
ae5c8be
2d4f8f3
485dae6
77d88a2
6fcafc7
2d53d5d
fdbf5ec
0e1b4b3
d2d8c45
a70debb
e01f262
17f8617
e14ad40
32049b0
bfe3994
7ff8a96
7f08d80
27ab961
79ef920
5e9cf8d
c02e74f
fa41eb8
753d312
b1b405c
22d613c
ae19a68
6913a52
05051e1
d4772c6
d821494
9d23552
5037da6
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.
Probably should say “DeSoto single diode model” instead of “single diode equation”.
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.
Could we add a function parameter
root_kwargs
which accepts a dictionary of arguments to pass on toroot
?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.
It's probably worth mentioning that there are no restrictions on the fit variables, i.e., series resistance could go negative.
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 you are touching this text, "correpsonding " is misspelled.
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 don't readily understand what this note means. I always thought the missing
_ref
was merely the legacy of an original oversight. @cwhanse Do you have an insight here?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.
R_s
is used to remain consistent with SAM variable names. Let's remove the "Note that.." text.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.
Key is actually named
optimize_result
. Don't lump this in with the rest of parameters, see comment below.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.
Unfortunately, including the
optimize_result
right in this dictionary makes chaining computations more difficult:There are at least a few options here:
optimize_result
.optimize_result
the second element in a returned tuple.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 apologize, I overlooked this change. If we are returning the
result
object, return as 2nd element in a tuple.optimize_result
would be a better variable name.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.
Sorry, I made this change too quickly.
@markcampanelli I think your view on the package is more complete than mine, so I will include the optimize_result as you suggested. (after all, the goal of this pull request is also to learn from experienced users/contributors of pvlib-python)
@cwhanse Why do you prefer to return it in a tuple? Intuitively I would have use the dict (3rd option of markcampanelli) for improving the readability in the later use of the output.
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.
so that it can be omitted if not of interest, e.g.,
params, _ = fit_sdm_desoto(
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.
Like option 2. I don't like option 3.
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.
Thanks for chiming in @cwhanse and @wholmgren. I'm good with option 2. Interestingly, @tylunel, I use option 3 in my own APIs (I always return a single dictionary, nested if necessary), but for some reason I guessed that option 2 would be more preferable than option 3 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.
move
cells_in_series
to theget_test_specs_params
fixture.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.
cells_in_series is not in the get_test_specs_params fixture because it changes at line 129 of the test in order to cause the RuntimeError. I feel like it would be more complicated to add it the fixture and to change it later to have the RuntimeError. What do you think ?
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'd prefer to see it added to the fixture and then modified in the function that causes the
RuntimeError
. That way the fixture returns a complete, consistent specification.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.
Having to delete this entry here suggests that it shouldn't be lumped in with the rest of the parameters.