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.

Sign up for GitHub

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

Jump to bottom

Proposal: split up pvlib/ivtools/sdm.py #2252

Open
kandersolar opened this issue Oct 10, 2024 · 4 comments · May be fixed by #2256
Open

Proposal: split up pvlib/ivtools/sdm.py #2252

kandersolar opened this issue Oct 10, 2024 · 4 comments · May be fixed by #2256
Milestone
v0.11.2

Comments

@kandersolar
Copy link
Member

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 (WIP: Convert between CEC and PVsyst single diode models #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:

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?
@echedey-ls
Copy link
Contributor

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.

@cwhanse
Copy link
Member

cwhanse commented Oct 10, 2024

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.

@kandersolar
Copy link
Member Author

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
Copy link
Member Author

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

@kandersolar kandersolar linked a pull request Oct 11, 2024 that will close this issue
Open
6 tasks
@kandersolar kandersolar added this to the v0.11.2 milestone Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.11.2
Development

Successfully merging a pull request may close this issue.

Split up pvlib.ivtools.sdm into a subpackage organized by SDM kandersolar/pvlib-python
3 participants
@cwhanse @kandersolar @echedey-ls