preconfigured ModelChain for pvwatts, sapm #1022
Conversation
|
@cwhanse are you ok with the pattern drafted here? |
|
Yes, I prefer the factory to the subclass but not strongly. |
wholmgren
changed the title
|
@cwhanse can you provide any additional default options for the pvsyst configuration? Let me know if I missed anything for sapm, too. |
pvlib/modelchain.py
Outdated
| losses_model: pvwatts_losses | ||
| """ | ||
|
|
||
| kwargs.update(PVWATTS_CONFIG) |
There was a problem hiding this comment.
If there is overlap in kwargs, this prioritizes PVWATTS_CONFIG, e.g. if the user passes transposition_model='isotropic', 'perez' will get used anyway. Should these lines should be config = PVWATTS_CONFIG.copy(); config.update(kwargs) so that user-supplied parameters take precedence over the defaults? Or maybe if people want to mix and match models, they should just use the main ModelChain constructor.
There was a problem hiding this comment.
Thanks. Yes and yes. I'll change the code, document the behavior, and encourage use of the main constructor for maximum flexibility.
|
@cwhanse this is ready for review. I suspect we'll need to modify the default configurations. |
|
anything else on this PR @cwhanse? |
wholmgren
changed the title
docs/sphinx/source/api.rstfor API changes.docs/sphinx/source/whatsnewfor all changes. Includes link to the GitHub Issue with:issue:`num`or this Pull Request with:pull:`num`. Includes contributor name and/or GitHub username (link with:ghuser:`user`).