Better Memory Management #43

bmeyers · 2017-01-26T00:44:29Z

Addressing issues #34 and #35

* don't make a copy of every string, module and object, instead use the same object * TODO: need to make copies as needed in `setSuns()` see SunPower#34

* use copy instead of deepcopy reduce unnecessary new objects * make new strings and modules if irradiance is dictionary * make new cells if given list of cells and irradiance, if given new irradiances for all cells or new irradiances for specified cells

* fix make new cells if given all irradiance, only assign new object to a list

* remove `make_new` * add test * always copy cells even if scalar

* move comment about cells/modules back to where it was originally

…ops - duh

…strings

…setting a whole module with one Ee.

…essary.

…ell ids and Ee values.

bmeyers · 2017-01-26T00:46:17Z

additional improvements on Mark's work:

Allow for string Ee dictionary keys
Only iterate over unique cells when setting entire module to one Ee
Only copy when necessary when setting subset of cells in a module with scalar Ee
Only copy when necessary when setting suns on a list of cells with a list of Ee values

bmeyers · 2017-01-26T00:47:01Z

Object sizes for 20 string by 20 module system with PCT492 cells, npts=1001:

Fresh system: 411kB
Application of test shade after Mark's improvements: 402.2MB
Application of test shade after Bennet's improvements: 63.1MB

For reference, before the changes made in gh34-35 a fresh system of this size would take >10GB.

Finally, nosetests ran in 5.12 seconds on my computer before the changes in this pull request. Now, they run in 3.88 sec.

bmeyers · 2017-01-26T00:51:38Z

@mikofski So, what's the verdict? This seem good?

mikofski

Yes, I think it great! Just 3-4 requests and it's good:

please remove that annoying # gh34: ... comment everywhere
change set(self.pvcells) to old_pvcells.itervalues(), should be faster, IMO cleaner
add TODO to case where user uses one scalar Ee value to set the entire module, that this is unoptimize and will create a lot of unnecessary cells, but we can pursue it in a future PR
add a test to show that if the user sets the sun on multiple cells to different irradiance, that extra cells are not created unnecessarily in this particular example (or something like it)
```
 # set str X, mod Y cells 2 & 3 to 33%, and cell 17 to 99%, should create 2 new objects only
 pvsys.setSuns({X: {Y: {'Ee': (0.33, 0.33, 0.99), 'cells': (2, 3, 17)}}})
```

add TODO to implement cleaner interface for case in 4 - ie: the same could be done with

 # possible "cleaner" alternative by grouping cells into tuples that match the set irradiance
 pvsys.setSuns({X: {Y: {'Ee': (0.33, 0.99), 'cells': [(2, 3), 17]}}})

mikofski · 2017-01-26T01:05:38Z

pvmismatch/pvmismatch_lib/pvmodule.py

            pvcells = PVcell(pvconst=self.pvconst)
        if isinstance(pvcells, PVcell):
-            pvcells = [copy(pvcells) for _ in xrange(self.numberCells)]
+            # GH35: don't make copies, use same reference for all objects


can we remove these comments now?

mikofski · 2017-01-26T01:08:04Z

pvmismatch/pvmismatch_lib/pvmodule.py

+                    else:
+                        new_pvcells[cell_id] = old_pvcells[pvcell]
+                self.pvcells = new_pvcells
+                for pvc in set(self.pvcells):


actually, I think set(self.pvcells) is just old_pvcells.itervalues() right? I think that's better, probably faster (by a femto second) since the set is already calculated and it's an iterator

I would consider set(self.pvcells) to be "cleaner" than old_pvcells.itervalues() because it makes it immediately clear that you are iterating over the current unique cells associated with the module. But I don't feel that strongly about it, and I see the point about it being marginally faster. I can change it.

Good point, but I think we should still change it, in favor of a something that's already been evaluated, If the name is the issue, we can rename first to make it clearer:

pvcell_set = old_pvcells.itervalues() # iterator over new copies of unique cells for pvc in pvcell_set: do stuff ...

mikofski · 2017-01-26T01:08:33Z

pvmismatch/pvmismatch_lib/pvmodule.py

-                    pvc.Ee = Ee_idx
+                self.pvcells = copy(self.pvcells)  # copy list first
+                for cell_idx, Ee_idx in enumerate(Ee):
+                    # gh34: make new objects as needed from copies


pls remove these "gh34 ..." comments, thx!

and can we add a TODO comment here to optimize this later. it pvmod.setSuns(Ee=0.1) maybe seldom used, but I think it will unnecessarily add a lot of new cells, that could possibly be identical. Probably the exact same technique as used else where to catalog the unique cells and their new copies could be applied here in the future. You could put that in the TODO too.

Makes sense

bmeyers · 2017-01-26T02:04:26Z

10-4, will do

bmeyers · 2017-01-26T20:53:50Z

did not mean to close the PR, that was an accident!

bmeyers · 2017-02-02T04:57:20Z

@mikofski I'm not sure I understand request 3?

add TODO to case where user uses one scalar Ee value to set the entire module, that this is unoptimize and will create a lot of unnecessary cells, but we can pursue it in a future PR

I thought the case where one scalar Ee value is used to set the entire module was already optimized. Are you talking about this section of pvmodule.py?

    def setSuns(self, Ee, cells=None):
        """
        Set the irradiance in suns, Ee, on the solar cells in the module.
        Recalculates cell current (Icell [A]), voltage (Vcell [V]) and power
        (Pcell [W]) as well as module current (Imod [A]), voltage (Vmod [V])
        and power (Pmod [W]).

        Args:
            Ee (:class:`numpy.ndarray`): Effective Irradiance [suns]
            cells (list): Cells to change [Optional]
        """
        if cells is None:
            if np.isscalar(Ee):
                new_pvcells = range(self.numberCells)  # new list of cells
                old_pvcells = dict.fromkeys(self.pvcells)  # same as set(pvcells)
                for cell_id, pvcell in enumerate(self.pvcells):
                    if old_pvcells[pvcell] is None:
                        new_pvcells[cell_id] = copy(pvcell)
                        old_pvcells[pvcell] = new_pvcells[cell_id]
                    else:
                        new_pvcells[cell_id] = old_pvcells[pvcell]
                self.pvcells = new_pvcells
                pvcell_set = old_pvcells.itervalues()
                for pvc in pvcell_set:
                    pvc.Ee = Ee

… different irradiance, that extra cells are not created unnecessarily

bmeyers · 2017-02-02T05:06:38Z

@mikofski All done except the TODO in request three!

mikofski · 2017-02-02T07:24:12Z

I think it's this section in pvstrings.py:

except AttributeError:
    # Ee was a list? just take first item in list
    if len(Ee) > 1:
        raise TypeError('Irradiance, Ee, should be scalar or dict')
    for mod_id, pvmod in enumerate(self.pvmods):
        self.pvmods[mod_id] = copy(pvmod)
        self.pvmods[mod_id].setSuns(Ee[0])

Not sure what I was thinking here. does this make extra unnecessary cells or modules?

In general I don't like this section, why is this allowed as input? I think if user puts in `pvstring.setSuns([blah]) then pvmismatch should just puke.

mikofski · 2017-02-02T07:47:15Z

There is an issue or issues I can't resolve. If I run the example in the documentation quickstart with v2.3 I get a different answer than when I run it with this branch:

In [1]: from pvmismatch import *
In [2]: pvsys = pvsystem.PVsystem(numberStrs=2, numberMods=8)
In [3]: pvsys.setSuns({0: {0: [(0.2, ) * 8, (11, 12, 35, 36, 59, 60, 83, 84)]}})
In [4]: pvsys.Vmp
Out[4]: 402.21758063400114
>>> for pvstr in pvsys.pvstrs:
...     for pvmod in pvstr.pvmods:
...         for n, pvc in enumerate(pvmod.pvcells):
...             if n in (11, 12, 35, 36, 59, 60, 83, 84):
...                 pvc.Tcell = 100. + 273.15  # [K] hot cells in RBD
...             else:
...                 pvc.Tcell = 50. + 273.15  # [K] normal cells
...         (pvmod.Imod, pvmod.Vmod, pvmod.Pmod,
...          pvmod.Isubstr, pvmod.Vsubstr) = pvmod.calcMod()  # update modules
...     (pvstr.Istring, pvstr.Vstring,
...      pvstr.Pstring) = pvstr.calcString()  # update each string in the system
>>> pvsys.Isys, pvsys.Vsys, pvsys.Psys = pvsys.calcSystem()
>>> (pvsys.Imp, pvsys.Vmp, pvsys.Pmp,
...  pvsys.Isc, pvsys.Voc, pvsys.FF, pvsys.eff) = pvsys.calcMPP_IscVocFFeff()

v2.3

In [6]: pvsys.Vmp
Out[6]: 366.47468039417822

bm-gh34-35

In [6]: pvsys.Vmp
Out[6]: 370.60261850573403

about 1% difference. I don't think it's round off error, but I haven't had a chance to dig into it deeper.

Should I merge this PR then open a new issue for this bug?

mikofski · 2017-02-02T08:57:24Z

OK, I figured it out. The "example" in the docs is wrong. It iterates over strings and modules, but it should only be changing the cell temperatures in string 0, module 0, not all of them. The version in this PR actually get's it correct, by accident though, because even though it does change the wrong cell temperatures initially, it changes them back since all of those cells refer to the exact same object, so and the so the 96th cell is the only one that get's set, except in string 0, module 0 which have an additional object for the cells that have 0.2 suns.

mikofski · 2017-02-02T09:31:34Z

Looks good. merging. I'll make any changes from my last comments before pushing a new release. I also want to update the docs and add a release robot. I think I'll bump it up to v3.0. Any suggestions what the release name should be - regional food alliteration with the letter "K" like Katmandu Ketchup? Karachi Kebobs? Kentucky Kiwi?

bmeyers · 2017-02-02T22:26:57Z

Sweet!!!

I like Kathmandu Ketchup

mikofski and others added 14 commits January 23, 2017 20:33

resolves issue SunPower#35: don't make copies

0aaf3fb

* don't make a copy of every string, module and object, instead use the same object * TODO: need to make copies as needed in `setSuns()` see SunPower#34

make new modules in string even if irradiance is scalar

68e3fd2

* fix make new cells if given all irradiance, only assign new object to a list

always make new objects

3154791

* remove `make_new` * add test * always copy cells even if scalar

remove comment legacy code

c113a41

* move comment about cells/modules back to where it was originally

copy list of modules first

ad14c33

copy list of cells first, log unique obj id numbers in test

a13ed8b

also copy cells first if changing all cell irradiance

2b451d5

fix if string Ee is a list of one Ee, move copy pvmods list before lo…

6517a4a

…ops - duh

Added allowance for Ee dictionary keys that are numbers formatted as …

f65f5ab

…strings

Easy efficiency improvement by only iterating over unique cells when …

131db4f

…setting a whole module with one Ee.

Setting subset of cells in a module with scalar Ee only copies as nec…

df67e45

…essary.

Same improvement on when to make copies for case when given list of c…

9b4108c

…ell ids and Ee values.

Use set instead of np.unique.

87609a1

bmeyers mentioned this pull request Jan 26, 2017

Better memory #42

Closed

mikofski suggested changes Jan 26, 2017

View reviewed changes

bmeyers closed this Jan 26, 2017

bmeyers reopened this Jan 26, 2017

bmeyers added 2 commits February 1, 2017 20:47

Removing gh34 comments.

8df6386

Replacing set(self.pvcells) with old_pvcells.itervalues()

cbc0b23

bmeyers added 2 commits February 1, 2017 20:58

Adding TODO for better setSun interface

2ea7540

add a test to show that if the user sets the sun on multiple cells to…

54e0719

… different irradiance, that extra cells are not created unnecessarily

mikofski approved these changes Feb 2, 2017

View reviewed changes

mikofski merged commit 6eee098 into SunPower:master Feb 2, 2017

bmeyers deleted the bm-gh34-35 branch February 2, 2017 22:56

mikofski mentioned this pull request Jan 17, 2018

cache last X calculations, and be smarter about checking cache before recalculating #60

Open

Better Memory Management #43

Better Memory Management #43

Uh oh!

Conversation

bmeyers commented Jan 26, 2017

Uh oh!

bmeyers commented Jan 26, 2017

Uh oh!

bmeyers commented Jan 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmeyers commented Jan 26, 2017

Uh oh!

mikofski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmeyers commented Jan 26, 2017 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmeyers commented Jan 26, 2017

Uh oh!

bmeyers commented Feb 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmeyers commented Feb 2, 2017

Uh oh!

mikofski commented Feb 2, 2017

Uh oh!

mikofski commented Feb 2, 2017

v2.3

bm-gh34-35

Uh oh!

mikofski commented Feb 2, 2017

Uh oh!

mikofski commented Feb 2, 2017

Uh oh!

bmeyers commented Feb 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bmeyers commented Jan 26, 2017 •

edited

Loading

bmeyers commented Jan 26, 2017 via email •

edited

Loading

bmeyers commented Feb 2, 2017 •

edited

Loading