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 34 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.
The
py35_min
environment purposefully omits scipy. To get the tests to complete, move thefrom scipy import …
statements intofit_sdm_desoto
. You could include atry/except
clause like here to more gently handle ImportErrors.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.
Have you attempted to compute the Jacobian of
pv_fct
that could be passed as thejac
parameter toroot
?This might be both faster and more robust, but it also can easily be left as a followup improvement.
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.
Not necessarily true. I believe the secant method is more stable (*) than the Newton-Raphson, and even though it converges slower, it might still be faster, if the Jacobian is computationally expensive to calculate.
(*) I believe secant or Broyden's is less sensitive to poor initial guess, discontinuity, and relatively flat or stiff systems, which can cause N-R to get stuck in an infinite loop, but I can't find a good reference for this, so maybe I'm wrong, sorry
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.
BTW, if you do need the derivatives, check
singlediode.py
they're there inbishop88()
by settinggradients=True
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.
Agree, let's tackle that with a follow-on PR.
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.
Maybe include the full
result
here in the exception for better debugging?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.
Since we're not exposing most of the scipy.optimize.root input options, I'm not in favor of returning the
OptimizeResult
object. We could addOptimizeResult.message
to the error message.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 personally like to be able to inspect the full
result
object from the solver (possibly as a second output in a returned tuple), but I also understand if the majority would rather omit it from the return interface.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.
Ok.
Then do you think it is worth testing the result object in the test ? I feel like some attribute of the complete OptimizeResult object can easily change (like nit the number of iteration, or nfev) if scipy is improved. That's why I would remove the optimizeresult from the dict output in test_fit_sdm_desoto() before testing the equivalence with expected result.
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 think it is worth testing the content of
result
in the CI tests.However, your question led me to look at the default settings for
solver_method in 'lm', 'hybr '
. The default settings terminate based on relative change of the solution, so it is possible that the root finder stalls in some flat portion of the objective surface rather than finding the minimum. We could build a function to test the quality of the solution thatroot
finds (e.g. compute an IV curve and compare points with the input data), but let's do that in a follow-on PR.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.
Could check that the exception raised is the exception we're after. Could do it like this (learned from @wholmgren quite recently):