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.

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

refactor ModelChain with methods instead of classes #194

Merged
merged 22 commits into from
Jul 27, 2016

Conversation

wholmgren
Copy link
Member

I proposed refactoring ModelChain into a base class, a SAPM class, and a SingleDiode class in #179. This likely is a bad idea. It severely limits the flexibility of future ModelChains by promoting a single model choice, the DC power model, to the level of a class choice. It is also likely to make user code unnecessarily complicated.

Here, I propose a new way to extend ModelChain that may be more flexible, more powerful, and easier to use. The strategy is to assign predefined or user-defined functions to standard method names. For example, ModelChain.dc_model will be set to ModelChain.sapm or ModelChain.singlediode. This is basically a controlled version of "monkey patching", which is generally regarded as a bad thing in Python. I'm not sure what the problems would be, though.

Here's a gist with a notebook that demonstrates the usage:

https://gist.github.com/wholmgren/3fabacd3003c2a549b420b5e79f55e95

Some highlights:

times = pd.date_range(start='20160101 0000-0700', end='20160104 0000-0700', freq='5min')
location = Location(32.2, -111, altitude=700)

# sapm model, just like in 0.3
# uses ModelChain.infer_dc_parameters to automatically determine which model to use
system = PVSystem(module_parameters=get_sapm_module_parameters(),
                  inverter_parameters=get_cec_inverter_parameters())
mc = ModelChain(system, location)
mc.run_model(times)

# singlediode model with cec module parameters
system = PVSystem(module_parameters=get_cec_module_parameters(),
                  inverter_parameters=get_cec_inverter_parameters())
mc = ModelChain(system, location)
mc.run_model(times)

# user defined parameters and functions
module_parameters = {'a': 3, 'b': 0.001, 'c': 1, 'd': -0.05}
system = PVSystem(module_parameters=module_parameters)
# pvusa_mc_wrapper is a user-defined function. see gist for details
mc = ModelChain(system, location, dc_model=pvusa_mc_wrapper, ac_model=pvusa_ac_mc_wrapper)
mc.run_model(times)

@wholmgren wholmgren added this to the 0.4.0 milestone Jun 19, 2016
@wholmgren
Copy link
Member Author

I added modelchain options for pvwatts dc/ac/losses models, aoi models, and spectral models.

I've done a lot of small tests of the code and I think it's fairly robust, but I've not extensively tested it under real world conditions. It will be at least 4 weeks before I can push the code to its breaking point. Would you rather have only modestly tested modelchain code merged and released this week or better tested code in 4 weeks? I prefer to merge and release now and let you do the real world testing and file bug reports.

@jforbess
Copy link
Contributor

I suspect you'll get the best feedback if you release now, and I think we
are still at the stage where it is reasonable to expect possible bugs in
new features, so I vote release early and often!

On Wed, Jul 27, 2016 at 10:30 AM, Will Holmgren notifications@github.com
wrote:

I added modelchain options for pvwatts dc/ac/losses models, aoi models,
and spectral models.

I've done a lot of small tests of the code and I think it's fairly robust,
but I've not extensively tested it under real world conditions. It will be
at least 4 weeks before I can push the code to its breaking point. Would
you rather have only modestly tested modelchain code merged and released
this week or better tested code in 4 weeks? I prefer to merge and release
now and let you do the real world testing and file bug reports.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#194 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AH66AQ0YHB2Gr4zhVkdyGEoTG-m9ZkWuks5qZ5WrgaJpZM4I5OB7
.

@wholmgren
Copy link
Member Author

Thanks for your input, Jessica.

Closes #143.

@wholmgren wholmgren merged commit d1bbad1 into pvlib:master Jul 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants