Breaking out parts of pvsystem.py?

[markcampanelli]

Sorry if this is a duplicate, but has anyone considered breaking out the different module performance models in pvsystem.py into their own modules for better manageability?

For example, put the desoto model and sapm into their own modules.

[wholmgren]

@mikofski brought up something similar in #235.

I agree that the pvsystem.py module has grown to a difficult to mange length. Would you rather see the module broken up by model heritage (e.g. sapm.py, pvwatts.py, diode.py) or would you rather see it broken up by model type (e.g. dcpower.py, acpower.py, iam.py).

I've also considered moving PVSystem (and maybe SingleAxisTracker) into a separate module.

In any case, a subpackage might be preferable for API consistency and for keeping some kind of grouping for this related functionality. On the other hand, the Zen of Python says "flat is better than nested", so this too is up for debate.

[jforbess]

I think it makes a little more sense to break them out by model heritage, because I would tend to add more complicated modeling aspects in an sapm model, whereas if I just wanted to apply a few quick and dirty losses I might use pvwatts.

…

On Sun, Mar 4, 2018 at 8:45 AM, Will Holmgren ***@***.***> wrote: @mikofski <https://github.com/mikofski> brought up something similar in #235 <#235>. I agree that the pvsystem.py module has grown to a difficult to mange length. Would you rather see the module broken up by model heritage (e.g. sapm.py, pvwatts.py, diode.py) or would you rather see it broken up by model type (e.g. dcpower.py, acpower.py, iam.py). I've also considered moving PVSystem (and maybe SingleAxisTracker) into a separate module. In any case, a subpackage might be preferable for API consistency and for keeping some kind of grouping for this related functionality. On the other hand, the Zen of Python says "flat is better than nested", so this too is up for debate. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#436 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AH66AeDIewBlWKRj2-b8ZbtPDVA3K-aYks5tbBosgaJpZM4SbWhP> .

[cwhanse]

I'm not in favor of breaking pvsystem into subpackages by model name (SAPM, Desoto, etc.). I am in favor of restructuring pvsystem by modeling step, and writing wrapper functions where we don't currently use them (the iam functions, for example), then moving the wrapped functions into a subpackage. As an example why I don't favor a 'sapm' subpackage, I believe that sapm_celltemp is the only cell temperature model in pvlib, currently. We would be better off to have a celltemp function that wraps celltemp_sapm (and celltemp_faiman and others) and use kwargs, like we do with irradiance functions.

Some of the length in pvsystem can be reduced if I would ever get to writing pvlib.io.

[wholmgren]

Good points @jforbess and @cwhanse.

Looking only at the "easier" ideas...

I think the only io related functions are retrieve_sam and _parse_raw_sam_df. Moving those would be an improvement of 130 lines.

systemdef could probably be removed entirely. It's not used anywhere else in the library and I don't know why anyone would use it on its own. That's another 80 lines.

_golden_sect_DataFrame and _pwr_optfcn could be moved to a _optimize.py module and/or will be replaced. 80 lines.

PVSystem and LocalizedPVSystem are 570 lines and can be moved to e.g. api.py (or e.g. objects.py and imported into api.py).

All together, that's about 30% of the module (2500 lines).

[markcampanelli]

I was playing around with a project structure like this:

pvlib-python/
  data/
    weather/
      atmosphere_gold.csv
      atmosphere_gold.json
      atmosphere_gold.py (generates the gold files)
        ...
    dc_device_iv/
      lfm/ (loss factor model)
      sapm/
      sdm/
        desoto/
        pvsyst/
        sdm_gold.csv
        sdm_gold.json
        sdm_gold.py
    ...
  models/
    weather/
      atmosphere.py
      clearsky.py
      ...
    dc_device_iv/
      lfm/ (loss factor model)
        lfm.py
      sapm/
        sapm.py
      sdm/
        desoto/
          desoto.py
        pvsyst/
          pvsyst.py
        sdm.py
    ...
  pvlib/ (this holds the user-facing API "wrappers", named pvlib/ not api/ due to legacy considerations)
    __init__.py (Can this be set up so that a user importing a pvlib API module doesn't automatically import any peer modules that are "psuedo-private"?)
    pvsystem.py (Could be renamed, with much less content, and we could break up further into API modules.)
    ...
  test/
    weather/
      test_atmosphere.py
      test_atmosphere_gold.py
      test_clearsky.py
      ...
    dc_device_iv/
      lfm/ (loss factor model)
        test_lfm.py
      sapm/
        test_sapm.py
      sdm/
        desoto/
          test_desoto.py
        pvsyst/
          test_pvsyst.py
        test_sdm.py
        test_sdm_gold.py
    ...
  .gitattributes
  LICENSE
  ...

It "stutters" a bit, but I think using different names in the stuttering parts would be more distracting than helpful. This would be a typical import into an user-facing API module like pvlib-python/pvlib/pvsystem.py:

import pvlib-python.models.dc_device_iv.sdm.sdm as sdm
import pvlib-python.models.dc_device_iv.sdm.desoto.desoto as desoto

A user-facing API import into a user's code would look like:

import pvlib.pvsystem as pvsystem  # To reiterate this could be renamed and possibly broken up

[wholmgren]

@thunderfish24 thanks for the detailed proposal. Do you know of any python packages that use a similar structure, in particular separating out the core code at the root level? The closest that I can think of is matplotlib.

[mikofski]

This could be difficult packaging, see my recommend project layout and Python Package Authority. All items to be bundled in the pvlib package should be in pvlib-python/pvlib/ including tests and docs if you want them to be in the distribution. Only vendorized packages and scripts should be at the top level