Skip to content
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

refactor pvcell.PVcell.__setattr__ or constructor to only set Icell, Vcell, and Pcell only once #59

Closed
mikofski opened this issue Jan 16, 2018 · 0 comments · Fixed by #61

Comments

@mikofski
Copy link
Contributor

mikofski commented Jan 16, 2018

The pcell.PVcell() constructor sets Icell, Vcell, and Pcell to None which calls the __setattr__() overload which calls calcCells() and sets Icell, Vcell, and Pcell again!

The constructor calls to self.__setattr__(key, value) look like this:

setting attribute Rs to 0.004267236774264931
setting attribute Rsh to 10.01226369025448
setting attribute Isat1_T0 to 2.28618816125344e-11
setting attribute Isat2 to 1.117455042372326e-06
setting attribute Isc0_T0 to 6.3056
setting attribute aRBD to 0.0001036748445065697
setting attribute bRBD to 0.0
setting attribute VRBD to -5.527260068445654
setting attribute nRBD to 3.284628553041425
setting attribute Eg to 1.1
setting attribute alpha_Isc to 0.0003551
setting attribute Tcell to 298.15
setting attribute Ee to 1.0
setting attribute pvconst to <PVconstants(npts=101)>
setting attribute Icell to None
setting attribute Vcell to None
setting attribute Pcell to None

Since __setattr__() checks to see if pvconst attribute exists, therefore it doesn't calculate Icell, Vcell, and Pcell until the end, but then it sets them again, calling calcCells() again and again and again.

I think just move the pvconst setting in the constructor to the last position, or maybe add an _calc_now flag might be more explicit. something like:

_calc_now = False
def __init__(self, Rs=RS, Rsh=RSH, Isat1_T0=ISAT1_T0, Isat2=ISAT2,
             Isc0_T0=ISC0_T0, aRBD=ARBD, bRBD=BRBD, VRBD=VRBD_,
             nRBD=NRBD, Eg=EG, alpha_Isc=ALPHA_ISC,
             Tcell=TCELL, Ee=1., pvconst=PVconstants()):
    # user inputs
    self.Rs = Rs  #: [ohm] series resistance
    self.Rsh = Rsh  #: [ohm] shunt resistance
    self.Isat1_T0 = Isat1_T0  #: [A] diode one sat. current at T0
    self.Isat2 = Isat2  #: [A] diode two saturation current
    self.Isc0_T0 = Isc0_T0  #: [A] short circuit current at T0
    self.aRBD = aRBD  #: reverse breakdown coefficient 1
    self.bRBD = bRBD  #: reverse breakdown coefficient 2
    self.VRBD = VRBD  #: [V] reverse breakdown voltage
    self.nRBD = nRBD  #: reverse breakdown exponent
    self.Eg = Eg  #: [eV] band gap of cSi
    self.alpha_Isc = alpha_Isc  #: [1/K] short circuit temp. coeff.
    self.Tcell = Tcell  #: [K] cell temperature
    self.Ee = Ee  #: [suns] incident effective irradiance on cell
    self.pvconst = pvconst  #: configuration constants
    self.Icell = None  #: cell currents on IV curve [A]
    self.Vcell = None  #: cell voltages on IV curve [V]
    self.Pcell = None  #: cell power on IV curve [W]
    # set calculation flag
    self._calc_now = True  # overwrites the class attribute

def __setattr__(self, key, value):
    try:
        value = np.float64(value)
    except (TypeError, ValueError):
        pass
    super(PVcell, self).__setattr__(key, value)
    # after all attributes have been initialized, recalculate IV curve
    # every time __setattr__() is called
    if self._calc_now:
        Icell, Vcell, Pcell = self.calcCell()
        super(PVcell, self).__setattr__('Icell', Icell)
        super(PVcell, self).__setattr__('Vcell', Vcell)
        super(PVcell, self).__setattr__('Pcell', Pcell)

I think this is kinda relevant to #48, or at least the last bit where it says, "all calculations should wait until all new parameters are set," but I thought that was already mentioned in another issue. Maybe the other issue was #15? Anyway, the reason why I think this is relevant to that part of the issue is because if you need to delay calculation of the cell, then just set pvc._calc_now = False and it won't self update until you set it to True again.

But this issue isn't directly related to #48 which is more about creating a single update method that copies cells, modules or strings as needed when updating irradiance, cell-temperature, or any parameter, instead of having methods for each parameter, eg setsuns() and settemps() which are nearly identical and repeated in pvmodules and pvstrings. See the TODO in the code.

This is easy to do in the PVcell.update() method:

def update(self, **kwargs):
    """
    Update user-defined constants.
    """
    # TODO: use __dict__.update()
    # self.__dict__.update(kwargs)
    # TODO: check for floats and update IV curve
    # * __setattr__ already checks for floats
    # turn off calculation flag until all attributes are updated to save time
    self._calc_now = False
    for k, v in kwargs.iteritems():
        setattr(self, k, v)
    self._calc_now = True  # recalculate
mikofski added a commit to mikofski/PVMismatch that referenced this issue Jan 17, 2018
* 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
chetan201 pushed a commit that referenced this issue Feb 5, 2018
…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__
chetan201 pushed a commit that referenced this issue Feb 6, 2018
…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
rayhickey added a commit to rayhickey/PVMismatch that referenced this issue 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 issue 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
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant