Isat2fix #64
Merged
Isat2fix #64
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
February 6, 2018 18:09
mikofski
reviewed
Feb 7, 2018
| """ | ||
| Diode two saturation current at Tcell in amps. | ||
| """ | ||
| _Tstar = self.Tcell ** 3. / self.pvconst.T0 ** 3. # scaled temperature |
Contributor
There was a problem hiding this comment.
Cool! Nice work!!! Looks like a paper for you. But I think the exponent here should be 5/2 power, not third peer, right? You should consult with a cell researcher like Seung Rim. Also my bad code, you'll save a flop by doing division first then power. Ditto for Isat1.
Contributor
Author
There was a problem hiding this comment.
Thanks for taking a look Mark! It seems there are various versions of that formula floating around. I took it from
https://works.bepress.com/dr_adel72/78/
Anyhow, I think the Insel slides are more recent and probably a better way to go. But I will consult Seung regarding what'd be more appropriate. Adding references also seems like a good idea.
Now, I just need to make the travis tests pass!
Contributor
|
Also would be nice if you could cite your sources like pvlib does. It's okay to cite the Insel slides. They're from the 6th PVPMC workshop in Freiburg, Germany. @BenBourne was there. See section 3: PV modeling. The talk is called "30 years of PV modeling." |
Contributor
|
@chetan201 can you post an example of the Voc vs. T plot to show that it is corrected? |
Contributor
|
you'll need to put " fixes #65 " in your commit message to close the issue |
mikofski
suggested changes
Feb 7, 2018
Contributor
mikofski
left a comment
There was a problem hiding this comment.
please correct the typos in gen_coeffs and maybe add a test that the new gen_coeffs work, probably better than ever!
| if x0 is None: | ||
| isat1 = ISAT1_T0 # [A] | ||
| isat2 = ISAT2 | ||
| ISAT_T0 = ISAT_T0 |
Contributor
There was a problem hiding this comment.
typo right? should be:
isat2 = ISAT2_T0
| else: | ||
| isat1 = x0[0] | ||
| isat2 = x0[1] | ||
| ISAT_T0 = x0[1] |
Contributor
There was a problem hiding this comment.
another typo right? Should be same as it was before, isat2 = x0[1], no change.
| rs = x0[2] | ||
| rsh = x0[3] | ||
| x = np.array([np.log(isat1), np.log(isat2), np.sqrt(rs), np.sqrt(rsh)]) | ||
| x = np.array([np.log(isat1), np.log(ISAT_T0), np.sqrt(rs), np.sqrt(rsh)]) |
Contributor
There was a problem hiding this comment.
another typo, right? should be no change.
| if sol.success: | ||
| isat1 = np.exp(sol.x[0]) | ||
| isat2 = np.exp(sol.x[1]) | ||
| ISAT_T0 = np.exp(sol.x[1]) |
| rs = sol.x[2] ** 2.0 | ||
| rsh = sol.x[3] ** 2.0 | ||
| return (isat1, isat2, rs, rsh), sol | ||
| return (isat1, ISAT_T0, rs, rsh), sol |
| # Slope at Max Power Point | ||
| dpdv, jdpdv = two_diode.fdpdv( | ||
| isat1=isat1, isat2=isat2, rs=rs, rsh=rsh, ic=imp, vc=vmp, vt=vt | ||
| isat1=isat1, ISAT_T0=ISAT_T0, rs=rs, rsh=rsh, ic=imp, vc=vmp, vt=vt |
| # Shunt Resistance | ||
| frsh, jrsh = two_diode.fjrsh( | ||
| isat1=isat1, isat2=isat2, rs=rs, rsh=rsh, vt=vt, isc=isc | ||
| isat1=isat1, ISAT_T0=ISAT_T0, rs=rs, rsh=rsh, vt=vt, isc=isc |
| jvoc = np.stack(( | ||
| -jid1_voc[0], # d/disat1 | ||
| -jid2_voc[0], # d/disat2 | ||
| -jid2_voc[0], # d/dISAT_T0 |
| jmpp = np.stack(( | ||
| -jid1_mpp[0], # d/disat1 | ||
| -jid2_mpp[0], # d/disat2 | ||
| -jid2_mpp[0], # d/dISAT_T0 |
| assert(np.all(np.gradient(np.squeeze(Vmp_arr)) < 0)) | ||
| assert(np.all(np.gradient(np.squeeze(Voc_arr)) < 0)) | ||
| assert(np.all(np.gradient(Isc_arr) > 0)) | ||
|
|
Contributor
There was a problem hiding this comment.
Great test, I assume the theory is correct, that it should decrease monotonically? Is there a citation or way to show this from first principles?
Overall nice work! Thanks for undertaking this! Kudos! It's a real improvement, and IMHO deserves to be a major release. I think you should bump it up to v4.
FYI: Maybe next time use "Refactor" instead of "Find & Replace" because there were a lot of typos in the pvmismatch.contrib.gen_coeffs package?
Contributor
Author
There was a problem hiding this comment.
Yes! Still it was a terrible decision to use replace. I had to do a lot of back and forth to track it down. Lesson learned!
added 2 commits
February 7, 2018 15:09
Contributor
|
@bmeyers and @nzweibaum you should really take a look at this, I think it's a major improvement and solves an issue that both of you and Keith Johnston complained about often - in particular the abysmal low temperature performance of PVMismatch. It should work much better after this PR. |
Contributor
|
@chetan201 looks good to me, but can you do two more things:
$ git diff
diff --git a/pvmismatch/contrib/gen_coeffs/example.py b/pvmismatch/contrib/gen_coeffs/example.py
index bf9cf07..9ccc441 100644
--- a/pvmismatch/contrib/gen_coeffs/example.py
+++ b/pvmismatch/contrib/gen_coeffs/example.py
@@ -23,12 +23,12 @@ T0, E0 = 25.0, 1000.0
iec61853 = gen_coeffs.gen_iec_61853_from_sapm(gen_coeffs.PVMODULES[PROD_NAME])
iec61853['i_mp'] = iec61853['p_mp'] / iec61853['v_mp']
#isc0, alpha_isc = gen_coeffs.gen_sapm(iec61853)
-x, sol = gen_coeffs.gen_two_diode(ISC0, VOC0, IMP0, VMP0, NS, NP, T0)
-#x, sol = gen_coeffs.gen_two_diode(
-# iec61853['i_sc'], iec61853['v_oc'], iec61853['i_mp'],
-# iec61853['v_mp'], NS, NP, tc=TC, method='lm',
-# x0=(2.25e-11, 1.5e-6, 0.004, 10.0)
-#)
+# x, sol = gen_coeffs.gen_two_diode(ISC0, VOC0, IMP0, VMP0, NS, NP, T0)
+x, sol = gen_coeffs.gen_two_diode(
+ iec61853['i_sc'], iec61853['v_oc'], iec61853['i_mp'],
+ iec61853['v_mp'], NS, NP, tc=TC, method='lm',
+ x0=(2.25e-11, 1.5e-6, 0.004, 10.0)
+)
isat1, isat2, rs, rsh = x
pvc = pvcell.PVcell(
I'd like to see if the thanks! |
mikofski
approved these changes
Feb 8, 2018
Contributor
mikofski
left a comment
There was a problem hiding this comment.
everything looks good now, just apply the patch to pvmismatch.contrib.gen_coeffs.example and update the quickstart tutorial in the documents, or any other documentation if necessary. You can push new docs to the GitHub pages by copying the output from sphinx into the top level docs folder
Contributor
|
or maybe don't apply the patch, it's a judgement call - your judgment call actually - owner |
Contributor
Author
|
Relative % difference in Vmp vs TempC characteristics between using T^3 vs T^2.5 for Isat2 equation |
Contributor
|
Maybe compare the temp coefficient, beta Vmp at 50[C]? Which exponent yields a tempco that is closest to some other published value? In my gut, I feel the cubic dependence (T^3) seems the safest but I have no reason or proof to think so. Another, option, is to make a new constant somewhere, for the exponent, and then don't hardcode it, instead use the constant. Doesn't have to be exposed to user, but still makes it easier to change as developer, and easier to expose later on? Something like: # somewhere
ISAT2_T_EXP = 3 # Isat 2 temperature dependence exponent
# then later
_Tstar = (self.Tcell / self.pvconst.T0) ** ISAT2_T_EXP # scaled temperature |
Contributor
Author
|
This is a release candidate. No point procrastinating releasing this for other outstanding issues. |
Contributor
|
🎉 Yay! |
rayhickey
added a commit
to rayhickey/PVMismatch
that referenced
this pull request
Feb 26, 2018
* improve python 3 compatibility in pvsys, strs and mods, require future * change dict.iter*() to iter*(dic), fix import * in tests * fix two diode test for SymPy-1.1.1 * fixes SunPower#55 * replace string keys in expected_data with the actual symbol objects, so instead of {'ic': IC}, use {ic: IC} where ic is the SymPy symbol object * replace string substitutions in expressions with the actual SymPy symbols, so use didv.subs(vc + rs*ic, 'vd') instead of didv.subs('vc + rs * ic(vc)', 'vd') which is error prone anyway, because the string could change, how do you know what string to use? * leave these subs in for now, but they are not necessary, since vd = vc + ic*rs can evaluate just fine * when numerically evaluating expressions using evalf(subs=expected_data) substitute values for the symbol di_dv, NOT the string "Derivative(ic(vc), vc)" since this doesn't work anymore! * don't substitute for the string 'ic(vc)', instead use the SymPy symbols ic, since parentheses don't work in SymPy substitutions or evaluations anymore, and this is more explicit since ic was defined as f(vc) when it was initialized! * freeze versions in requirements so this doesn't an issue again * ignore .spyproject - the new spyder-IDE project file Signed-off-by: Mark Mikofski <bwana.marko@yahoo.com> * update travis to test Py3, update setup with correct install and test reqs, add setup.cfg for universal wheel * ENH: BUG: add _calc_now flag to determine when cell is recalculated (GH59) (SunPower#61) * fixes SunPower#59 * add class attr _calc_now * override _calc_now with instance attribte at the end of constructor * check if _calc_now is True to decide to call self.calcCell() instead of checking for pvconst * use self.__dict__.update() instead of using super(PVcell, self).__setattr__(key, value) * in update, remove TODO, b/c even tho __dict__.update() would bypass __setattr__() then would have to check for floats, and still set _calc_now = True to trigger recalc, so instead, just set _calc_now = False first to turn calculations off, until all attr are set, then recalc * don't set pvcell.pvconst in pvstring * just raise an exception if they don't match for now * remove comments about "deepcopy" everywhere * oops, recalculate means _calc_now = True, duh! * remove commented legacy code in __setattr__, add comment in update re checking for floats * add test for new _calc_now flag * if _calc_now == False, then calcCell() is not called in __setattr__ * if _calc_now == True, then calcCell() is called in __setattr__ * BUG: pvconst is not a singleton, inconsistent, doesn't update (GH38) (SunPower#62) * fixes SunPower#59 * add class attr _calc_now * override _calc_now with instance attribte at the end of constructor * check if _calc_now is True to decide to call self.calcCell() instead of checking for pvconst * use self.__dict__.update() instead of using super(PVcell, self).__setattr__(key, value) * in update, remove TODO, b/c even tho __dict__.update() would bypass __setattr__() then would have to check for floats, and still set _calc_now = True to trigger recalc, so instead, just set _calc_now = False first to turn calculations off, until all attr are set, then recalc * don't set pvcell.pvconst in pvstring * just raise an exception if they don't match for now * remove comments about "deepcopy" everywhere * oops, recalculate means _calc_now = True, duh! * remove commented legacy code in __setattr__, add comment in update re checking for floats * add test for new _calc_now flag * if _calc_now == False, then calcCell() is not called in __setattr__ * if _calc_now == True, then calcCell() is called in __setattr__ * closes SunPower#38 consistent pvconst behavior * use duck typing to check pvstrs in PVsystem() for list, object or none * set pvconst from pvstrs if given * set numbstrs from pvstrs if given * set numbermods form pvstrs.pvmods if given * check that pvconst and pvmods are consistent * add tests * apply same changes in pvsystem from last commit to pvstr * change default pvconst arg to None * use duck typing to check if pvmods is a list, an object, or None * relax requirement that all strings have same number of modules * change pvsys.numberMods to a list of number of modules in each string * add test to check pvstring * check that all pvconst are the same for all modules * add docstring for members * also test that pvsys.numberMods is now a list * each item in list is number of modules in corresponding string * use ducktyping to determine if pvcells is list or obj or None * add missing blank lines and wrap long lines per pep8 * add pvcell object to pvcells docstring arg type * add tests in test_module to check that pvconst is the same for module and all cells * add test for update method * replace npts attr with a property * add new private _npts attribute, return for npts in getter * set npts, pts, negpts, Imod_pts, and Imod_negpts in setter * add test to show that changing npts changes pts, negpts, Imod_pts, and Imod_negpts * add pvsystem update method * make system calculation DRY, use everywhere calcSystem and calcMPP_IscVocFFeff are called back to back to set Isys, Vsys, Psys, etc. * also makes it explicity to recalc the system after a change, like change pvconst.npts * Update example.py Isat2 => Isat2_T0 * reverting changes - didn't mean to commit master * Isat2fix (SunPower#64) * Added temperature dependance for Isat2, i.e. Isat2(Tcell) * test for wide range of Tcell values - added after implementing Isat2 as a function of Tcell * generated new iv curve * Isat2 to Isat2_T0 * Update example.py Isat2 => Isat2_T0 * Update test_diode.py * Deleting the old IV curve * updated IV curve test file * fixed isat2 typos caused by replace * use entire iec matrix * Update .travis.yml added new pipy credentials
chetan201
pushed a commit
that referenced
this pull request
Mar 21, 2018
* Merging v4 branch before submitting PR (#1) * improve python 3 compatibility in pvsys, strs and mods, require future * change dict.iter*() to iter*(dic), fix import * in tests * fix two diode test for SymPy-1.1.1 * fixes #55 * replace string keys in expected_data with the actual symbol objects, so instead of {'ic': IC}, use {ic: IC} where ic is the SymPy symbol object * replace string substitutions in expressions with the actual SymPy symbols, so use didv.subs(vc + rs*ic, 'vd') instead of didv.subs('vc + rs * ic(vc)', 'vd') which is error prone anyway, because the string could change, how do you know what string to use? * leave these subs in for now, but they are not necessary, since vd = vc + ic*rs can evaluate just fine * when numerically evaluating expressions using evalf(subs=expected_data) substitute values for the symbol di_dv, NOT the string "Derivative(ic(vc), vc)" since this doesn't work anymore! * don't substitute for the string 'ic(vc)', instead use the SymPy symbols ic, since parentheses don't work in SymPy substitutions or evaluations anymore, and this is more explicit since ic was defined as f(vc) when it was initialized! * freeze versions in requirements so this doesn't an issue again * ignore .spyproject - the new spyder-IDE project file Signed-off-by: Mark Mikofski <bwana.marko@yahoo.com> * update travis to test Py3, update setup with correct install and test reqs, add setup.cfg for universal wheel * ENH: BUG: add _calc_now flag to determine when cell is recalculated (GH59) (#61) * fixes #59 * add class attr _calc_now * override _calc_now with instance attribte at the end of constructor * check if _calc_now is True to decide to call self.calcCell() instead of checking for pvconst * use self.__dict__.update() instead of using super(PVcell, self).__setattr__(key, value) * in update, remove TODO, b/c even tho __dict__.update() would bypass __setattr__() then would have to check for floats, and still set _calc_now = True to trigger recalc, so instead, just set _calc_now = False first to turn calculations off, until all attr are set, then recalc * don't set pvcell.pvconst in pvstring * just raise an exception if they don't match for now * remove comments about "deepcopy" everywhere * oops, recalculate means _calc_now = True, duh! * remove commented legacy code in __setattr__, add comment in update re checking for floats * add test for new _calc_now flag * if _calc_now == False, then calcCell() is not called in __setattr__ * if _calc_now == True, then calcCell() is called in __setattr__ * BUG: pvconst is not a singleton, inconsistent, doesn't update (GH38) (#62) * fixes #59 * add class attr _calc_now * override _calc_now with instance attribte at the end of constructor * check if _calc_now is True to decide to call self.calcCell() instead of checking for pvconst * use self.__dict__.update() instead of using super(PVcell, self).__setattr__(key, value) * in update, remove TODO, b/c even tho __dict__.update() would bypass __setattr__() then would have to check for floats, and still set _calc_now = True to trigger recalc, so instead, just set _calc_now = False first to turn calculations off, until all attr are set, then recalc * don't set pvcell.pvconst in pvstring * just raise an exception if they don't match for now * remove comments about "deepcopy" everywhere * oops, recalculate means _calc_now = True, duh! * remove commented legacy code in __setattr__, add comment in update re checking for floats * add test for new _calc_now flag * if _calc_now == False, then calcCell() is not called in __setattr__ * if _calc_now == True, then calcCell() is called in __setattr__ * closes #38 consistent pvconst behavior * use duck typing to check pvstrs in PVsystem() for list, object or none * set pvconst from pvstrs if given * set numbstrs from pvstrs if given * set numbermods form pvstrs.pvmods if given * check that pvconst and pvmods are consistent * add tests * apply same changes in pvsystem from last commit to pvstr * change default pvconst arg to None * use duck typing to check if pvmods is a list, an object, or None * relax requirement that all strings have same number of modules * change pvsys.numberMods to a list of number of modules in each string * add test to check pvstring * check that all pvconst are the same for all modules * add docstring for members * also test that pvsys.numberMods is now a list * each item in list is number of modules in corresponding string * use ducktyping to determine if pvcells is list or obj or None * add missing blank lines and wrap long lines per pep8 * add pvcell object to pvcells docstring arg type * add tests in test_module to check that pvconst is the same for module and all cells * add test for update method * replace npts attr with a property * add new private _npts attribute, return for npts in getter * set npts, pts, negpts, Imod_pts, and Imod_negpts in setter * add test to show that changing npts changes pts, negpts, Imod_pts, and Imod_negpts * add pvsystem update method * make system calculation DRY, use everywhere calcSystem and calcMPP_IscVocFFeff are called back to back to set Isys, Vsys, Psys, etc. * also makes it explicity to recalc the system after a change, like change pvconst.npts * Update example.py Isat2 => Isat2_T0 * reverting changes - didn't mean to commit master * Isat2fix (#64) * Added temperature dependance for Isat2, i.e. Isat2(Tcell) * test for wide range of Tcell values - added after implementing Isat2 as a function of Tcell * generated new iv curve * Isat2 to Isat2_T0 * Update example.py Isat2 => Isat2_T0 * Update test_diode.py * Deleting the old IV curve * updated IV curve test file * fixed isat2 typos caused by replace * use entire iec matrix * Update .travis.yml added new pipy credentials * Fixes MPP Sensitivity to Temperature Fixes I_mp and V_mp sensitivity to temperature caused by selecting Isys and Vsys values at the maximum Psys index. This is accomplished by linear interpolation between the points surrounding MPP. * Correct indices * Cleanup * Changed setsuns test for new MPP calculation Also changed MPP calc to return float rather than array
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Added Isat2 dependance on Tcell to fix undesirable Tcell vs Voc and Tcell vs Vmp characteristics.