pvlib-python icon indicating copy to clipboard operation
pvlib-python copied to clipboard

Proposal: split up `pvlib/ivtools/sdm.py`

Open kandersolar opened this issue 1 year ago • 4 comments

pvlib/ivtools/sdm.py is currently rather lengthy at ~1350 lines of code. Near-term additions (#2212 and #2185) will bring it to ~2000. I suggest we split up sdm.py into submodules, similar to what was recently done in pvlib.spectrum (#2125).

Current and future functionality spans:

  • estimating parameter values for various SDMs using various data sources
  • converting parameter values from one SDM to another (#2212)
  • calculating derived values (PVsyst temp coeff)
  • calculating SDE parameters (if we move the calcparams_XX functions here)

How should it be split up? As a starting point, one idea is to organize the parameter fitting functions according to the data source:

  • sdm/_fit_datasheet.py: fit_cec_sam, fit_desoto
  • sdm/_fit_ivcurves.py: fit_pvsyst_sandia, fit_desoto_sandia
  • sdm/_fit_iec61853: #2185
  • sdm/_convert.py: #2212
  • sdm/_misc.py: pvsyst_temperature_coeff

This division has the advantage of keeping fit_pvsyst_sandia and fit_desoto_sandia together, which is nice because they share a lot of code.

Questions:

  • Should the calcparams_XX functions be moved to pvlib.ivtools.sdm?
  • Does this division accommodate future functionality additions?

kandersolar avatar Oct 10 '24 15:10 kandersolar

I love that idea. Can we drop the first underscore in filenames suggestions? Users importing from file namespace (instead of submodule through __init__.py) may think these are private files (weak internal use in PEP-8. Source.)

Should the calcparams_XX functions be moved to pvlib.ivtools.sdm? Given they are currently in pvlib.pvsystem, +1. But probably in a separate PR, or at least be extra careful with deprecations.

echedey-ls avatar Oct 10 '24 18:10 echedey-ls

Imagining myself as a user, it seems more natural to first look for a function that fit a specific model (e.g. CEC) and then filter by the kind of data available (IEC61853 or datasheet), which suggests ivtools.sdm.cec. As you point out, though, that pattern would require importing (from somewhere) a number of private/supporting functions that are used by both of the sandia methods.

cwhanse avatar Oct 10 '24 19:10 cwhanse

I was imagining that the user experience would not change; all functions would still be accessed and documented via pvlib.ivtools.sdm.<function_name>. Users need not even know about the underlying modules. Rather, the effect of the reorganization would be to improve the developer experience.

kandersolar avatar Oct 10 '24 19:10 kandersolar

After more thought, I am warming to the idea of organizing by model. I will open a PR for consideration.

kandersolar avatar Oct 11 '24 15:10 kandersolar