Skip to content

pre-configured ModelChains #1013

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

Closed
wholmgren opened this issue Jul 29, 2020 · 13 comments · Fixed by #1022
Closed

pre-configured ModelChains #1013

wholmgren opened this issue Jul 29, 2020 · 13 comments · Fixed by #1022
Labels
enhancement SPI DOE SETO Solar Performance Insight project
Milestone

Comments

@wholmgren
Copy link
Member

wholmgren commented Jul 29, 2020

Is your feature request related to a problem? Please describe.
ModelChain has a lot of options and can be challenging to configure. This also makes it difficult to implement reference implementations of workflows.

Describe the solution you'd like
Create wrappers for pre-configured ModelChains. For example, a PVWatts version of ModelChain might look like:

# modelchain.py
PVWatts = functools.partial(
    ModelChain, 
    dc_model='pvwatts', ac_model='pvwatts', losses_model='pvwatts', 
    transposition_model='perez', aoi_model='physical'
)

# user code
my_model_chain = PVWatts(my_location, my_system)

This SO post discusses some subtleties with using partial with classes, so the actual implementation might require the helper function in that post.

Describe alternatives you've considered
We could subclass ModelChain but that seems like overkill for this. Inheritance leads to brittle code, in my experience, so I prefer to avoid it if possible.

We could supply dicts of parameters e.g.

# modelchain.py
pvwatts_config = dict(
    dc_model='pvwatts', ac_model='pvwatts', losses_model='pvwatts', 
    transposition_model='perez', aoi_model='physical'
)

# user code
my_model_chain = ModelChain(my_location, my_system, **pvwatts_config)

Any other ideas?

@wholmgren wholmgren added enhancement SPI DOE SETO Solar Performance Insight project labels Jul 29, 2020
@cwhanse
Copy link
Member

cwhanse commented Jul 29, 2020

Replacing the multiple model name attributes with a single dict has some appeal...

@wholmgren
Copy link
Member Author

I guess the two ideas could also be combined:

# modelchain.py
pvwatts_config = dict(
    dc_model='pvwatts', ac_model='pvwatts', losses_model='pvwatts', 
    transposition_model='perez', aoi_model='physical'
)

PVWatts = functools.partial(ModelChain, **pvwatts_config)


# user code
my_model_chain = ModelChain(my_location, my_system, **pvwatts_config)
# or
my_model_chain = PVWatts(my_location, my_system)

@wholmgren
Copy link
Member Author

Any comments on preferred approaches from folks that use ModelChain and would like some additional standardization? @kanderso-nrel @mikofski do you use ModelChain and/or have opinions on this? Would something like this be useful for rdtools?

@kandersolar
Copy link
Member

I haven't used ModelChain in any significant way, but I mildly prefer offering a "partial class" over exposing the pvwatts_config dict for users to double-splat into their ModelChain(...) calls. Users not familiar with splatting might not find that interface any more appealing than the main ModelChain interface.

Would something like this be useful for rdtools?

I've wondered about this too. NREL/rdtools#173 added the ability to normalize against any user-supplied expected energy timeseries, which I think fits into a broader sentiment of "rdtools isn't in the PV modeling business". I don't see much of a benefit to rdtools interacting with a ModelChain instance and calling its methods instead of having the user do it and pass in the result instead. Maybe @mdeceglie has thoughts?

@ncroft-b4
Copy link
Contributor

To me the pre-configured dict alone is sufficient and a simple solution from a maintainer standpoint, but how would the dict be offered to users for use in their code? The simplest entrypoint for the user may very well be the PVWatts partial class.

Though I worry the standalone PVWatts is redundant and just creates extra entries into the chain to maintain, especially given the complications described in the SO.

There are some possible benefits to using the pre-configured dict with ModelChain such as maintaining compatibility where it is already being used and setting a precedent for other pre-configurations to follow the same format without needed other partial Classes for each one.

Would there be interest in adding a parameter to ModelChain like 'model_strategy' in the vein of orientation_strategy, that when set configures the pvwatts_config dict on the backend? So the user would call ModelChain(my_location, my_system, model_strategy='pvwatts')?

@wholmgren
Copy link
Member Author

Thanks for the feedback.

I started trying to implement the partial approach and realized that I was going to need to assign a doc string to PVWatts.__doc__. That had me thinking that maybe it wasn't the right pattern, so decided to go ahead with trying inheritance in #1022. I combined it with the pre-configured dict for variety. Not committed to this, just wanted to throw it out there.

how would the dict be offered to users for use in their code?

It would just be accessible as pvlib.modelchain.PVWATTS_CONFIG. And documented somewhere. This kind of thing is not very discoverable, though.

Would there be interest in adding a parameter to ModelChain like 'model_strategy' in the vein of orientation_strategy, that when set configures the pvwatts_config dict on the backend? So the user would call ModelChain(my_location, my_system, model_strategy='pvwatts')?

Interesting idea. I like the simplicity of the interface. I think it might be difficult to handle and to document how that keyword argument interacts with the other keyword arguments, but maybe doable. I have the same concern with orientation_strategy -- this option overwrites whatever I've specified in the PVSystem initialization so it doesn't feel intuitive to me.

@mikofski
Copy link
Member

I don't use the model chains, but I prefer the subclass approach in #1022.

@kandersolar
Copy link
Member

kandersolar commented Aug 11, 2020

Tossing out another idea, the factory pattern, perhaps used with @ncroft-b4's model_strategy parameter, something like pvlib.modelchain.make_chain(..., strategy='pvwatts'). Or a static method on the ModelChain class.

@wholmgren
Copy link
Member Author

wholmgren commented Aug 11, 2020

Factory pattern is a good idea. Location has a couple of factories like from_tmy, so it fits with the library. I think I'd go for a class method on the ModelChain class but don't feel strongly about it.

We might want a different method for each predefined chain because they won't necessarily have the same parameters. For example, a PVWatts chain would not expose transposition to the user (documentation says to use Perez) but another chain might leave that to the user as a kwarg.

I could see similar factories for PVSytem that help people avoid the less-than-explicit API of "put these keys in modeling_parameters and these keys in inverter_parameters".

(I fixed the link in @kanderso-nrel's post)

@wholmgren
Copy link
Member Author

PVWatts and SAPM factories implemented in #1022 for discussion.

@ncroft-b4
Copy link
Contributor

I think I slightly prefer the make_chain factories over the subclass approach in #1022. It leaves it more explicit that what the user is using is ModelChain and not something different. What a PVWatts is is not very clear, but a ModelChain...pvwatts is pretty explanatory.

There might be a simpler name for the method. Maybe instead of make_chain it could be ModelChain.with_pvwatts() and ModelChain.with_sapm() , etc? I don't feel strongly about that, just throwing it out there for thought.

@wholmgren
Copy link
Member Author

Good points @ncroft-b4.

@cwhanse
Copy link
Member

cwhanse commented Aug 12, 2020

In #1022, I like having the PVWATTS_CONFIG dict for maintenance. I prefer the ModelChain.with_pvwatts method over the subclass, but not strongly. I think the with_pvwatts style is more explicit and easier to understand than the subclass approach.

@wholmgren wholmgren added this to the 0.8.0 milestone Aug 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement SPI DOE SETO Solar Performance Insight project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants