Skip to content

BUG: Extend current in calcSeries to quad4#112

Merged
chetan201 merged 5 commits into
SunPower:masterfrom
mikofski:gh110_fix_pvsys_resolution
Oct 31, 2019
Merged

BUG: Extend current in calcSeries to quad4#112
chetan201 merged 5 commits into
SunPower:masterfrom
mikofski:gh110_fix_pvsys_resolution

Conversation

@mikofski
Copy link
Copy Markdown
Contributor

  • fixes "Resolution" of the I-V plots made with plotSys() changes with cell temperature #110 - bad resolution for IV curve at high temperature
  • change Iforward range to be from 0 to meanIsc, instead of from Imin
    which was TOO SMALL, and use pvconst.Imod_pts which has closer spacing
    near zero than pvconst.pts
  • add quad4 range from Imin to zero, where Imin is a negative current in
    the 4th quadrant, using pvconst.Imod_negpts which goes from 1 to 0,
    with closer spacing near zero
  • concatenate Iquad4 before Iforward, and increase Vtot to same size
  • also add new pvconst.Imod_pts_sq which has even closer spacing near
    zero to use with Ireverse, so there's more than 1 point before the
    breakdown voltage!
  • add pvstr.Voc_mod which is a list of the sum of Voc for all cells in
    each module
  • and add pvsys.Voc_str which is a list of the sum of the Voc for all
    modules in each string
  • then use pvsys.Voc_str.max() instead of pvsys.Vstring.max() in
    calcParallel so there's not too many points in the 4th quadrant pull
    thing the curve way off, and messing up the resolution in the forward
    bias region (1st quadrant)
  • add a test in test_pvconstants to test the minimum current close to
    max voc by comparing the IVP curve of pvsys with data from gh110.dat
    which contains the correct resolution for a curve with npts=101, the
    default at Tcell=288[K]
  • also update test_pvcell.test_calc_series() with new data, but also
    compare the old data by interpolating with the new - we can delete the
    old data eventually right?
  • still a few more bugs, tests to fix, so this is a WIP

- fixes SunPower#110 - bad resolution for IV curve at high temperature
- change Iforward range to be from 0 to meanIsc, instead of from Imin
which was TOO SMALL, and use pvconst.Imod_pts which has closer spacing
near zero than pvconst.pts
- add quad4 range from Imin to zero, where Imin is a negative current in
the 4th quadrant, using pvconst.Imod_negpts which goes from 1 to 0,
with closer spacing near zero
- concatenate Iquad4 before Iforward, and increase Vtot to same size
- also add new pvconst.Imod_pts_sq which has even closer spacing near
zero to use with Ireverse, so there's more than 1 point before the
breakdown voltage!
- add pvstr.Voc_mod which is a list of the sum of Voc for all cells in
each module
- and add pvsys.Voc_str which is a list of the sum of the Voc for all
modules in each string
- then use pvsys.Voc_str.max() instead of pvsys.Vstring.max() in
calcParallel so there's not too many points in the 4th quadrant pull
thing the curve way off, and messing up the resolution in the forward
bias region (1st quadrant)
- add a test in test_pvconstants to test the minimum current close to
max voc by comparing the IVP curve of pvsys with data from gh110.dat
which contains the correct resolution for a curve with npts=101, the
default at Tcell=288[K]
- also update test_pvcell.test_calc_series() with new data, but also
compare the old data by interpolating with the new - we can delete the
old data eventually right?
- still a few more bugs, tests to fix, so this is a WIP
- offset Imod_pts_sq by EPS so it doesn't start at exactly zero
- rearrange and reword pts spacings so Imods are togethers, and clarify
when exactly zero vs. close to zero by EPS or 0.1/NPTS
- change/revert Iforward so that spacing is closer to short circuit so
the first voltage point isn't so relatively large that it's closer
to Vmp and Voc than it is from zero
- add optional Voc term to calcParallel and break up IV curve into 3
parts similar to the way the cell splits up voltage, if optional Voc
term is used then voltages are [Vrbd->0, 0->Voc, Voc->Vmax]
- supply Voc term for PVconstants.calcParallel in
pvmodule.combine_parallel_circuits()
- get Voc for the cells in series as the sum of Voc
- get average Voc for parallel circuits as the average
- update test data, rename series cells test with _old
- add scripts to save data

Signed-off-by: Mark Mikofski <bwana.marko@yahoo.com>
@mikofski
Copy link
Copy Markdown
Contributor Author

OK, fixed partial cross ties, but now total cross ties is broken

- add pvconst param and returns to docstring for combine_parallel_circuits
- add Voc arg for tct in calcMod when calling calcParallel using average of cell Voc for the row
- iterate over parallel circuits in pct with bridges and interpolate to get Voc, to pass as arg into calcParallel
@mikofski
Copy link
Copy Markdown
Contributor Author

@adambgnr and @chetan201 I think this is ready for review.

@adambgnr please bear in mind that when I started pvmismatch like 7 years ago, I was just learning Python so there's a lot of really bad code in here that should probably be fixed up, but I don't think this PR covers that.

This PR just fixes #110 the high temperature system, string, and module problem. The bug was caused by #73 a fix for #36 which solved the high temperature problem for voltage in the cell. This fix is the same approach, but at the module, string, and system level, and splits the IV curve into 3 regions instead of 2, when interpolating either voltage or current:

  • quadrant 4
  • forward bias
  • reverse bias

Let me know if you have any questions, and please open separate issues if you want to address improving the code for readability or performance. Thanks!

Comment thread pvmismatch/pvmismatch_lib/pvsystem.py
@mikofski mikofski changed the title WIP/BUG: Extend current in calcSeries to quad4 BUG: Extend current in calcSeries to quad4 Sep 17, 2019
@mikofski
Copy link
Copy Markdown
Contributor Author

@chetan201 ready for review. @adambgnr feel free to add your comments, as you're the one who caught the bug.

Comment thread pvmismatch/pvmismatch_lib/pvconstants.py
@adambgnr
Copy link
Copy Markdown
Contributor

@chetan201 ready for review. @adambgnr feel free to add your comments, as you're the one who caught the bug.

Thanks a lot for making this fix @mikofski! Indeed now the plots (and the underlying results) are nice and smooth regardless of the temperature. While testing, I have found one thing that I don't understand, see in the comments at npts().

@mikofski
Copy link
Copy Markdown
Contributor Author

Here's what the different spacings are supposed to look like:
Figure_1

So using np.flip(Imod_negpts) would reverse the spacing to create tighter spacing near the max power point, and less spacing as Vsys -> Voc.

from pvmismatch import *
pvconst = pvconstants.PVconstants(npts=20)
pvsys = pvsystem.PVsystem(pvconst=pvconst)
f = pvsys.plotSys(fmt='+:')
f.show()

It looks like this:
Figure_2

I like it better. The benefit is that there are more points around the knee of the max power point which is kind of what we originally wanted so it's easier to determine the max power, and the only downside is that we may miss some curvature near Voc, but the user can increase npts if they want more detail there.

I also updated the plotSys(fmt='') to allow specification of plotting formats like "+:" used here, and I saved the new set of points in PVconstants as Vmod_q4pts. I noticed that Imod_pts isn't used anywhere anymore except to create Imod_pts_sq which is a really tight spacing to capture the reverse breakdown, which is very flat and fast for avalanche. Maybe there should be a ticket to remove this at some point, not sure if deprecation is required, since it should be private anyway. Also I added sharex and tight_layout() to the figures so they look nicer and zoom more easily.

- add PVconstants.Vmod_q4pts which has decreasing spacing from 0.1/npts to one
- use Vmod_q4pts for Vquad4 in pvcell.py
- use Vmod_q4pts for Vquad4 in calcParallel()
- make all plots tight_layout()
- sharex for pvsys, pvstr, and pvmod
- add fmt='' to pvsys.plotSys() to allow user to set plot formatters like "+:"
- update tests and testdata with new values
@adambgnr
Copy link
Copy Markdown
Contributor

It looks like this:
Figure_2

I like it better. The benefit is that there are more points around the knee of the max power point which is kind of what we originally wanted so it's easier to determine the max power, and the only downside is that we may miss some curvature near Voc, but the user can increase npts if they want more detail there.

I also updated the plotSys(fmt='') to allow specification of plotting formats like "+:" used here

I like it too! Also I like the new fmt='+:' option for plotSys()! Much more informative than just the line plot.

@chetan201
Copy link
Copy Markdown
Contributor

@mikofski @adambgnr
Thanks for catching and fixing this. Since it changes the absolute value of the MPP, I would like to review a few cases before I approve. Thanks for your patience!

@mikofski
Copy link
Copy Markdown
Contributor Author

mikofski commented Oct 23, 2019

@chetan201 - gentle nudge 😉

@chetan201 chetan201 merged commit 5c4bc75 into SunPower:master Oct 31, 2019
@mikofski mikofski deleted the gh110_fix_pvsys_resolution branch December 3, 2019 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"Resolution" of the I-V plots made with plotSys() changes with cell temperature

3 participants