Location demotion and PVSystem/LocalizedPVSystem/ModelChain addition

[wholmgren]

This code does not work.

This PR is only to make it easier to share and discuss the ideas originally proposed in #17. Edit: this will eventually be merged.

I'm going to flesh it out a bit more and provide some examples in the next week or so.

There are two main ideas in this PR:

1. Make the procedural code self contained. If you don't like objects, don't use them. get_solarposition(times, location) becomes get_solarposition(times, latitude, longitude) and similar changes for a few other functions.
2. Create a class structure. Want the solar position for a PVSystem? Call PVSystem.get_solarposition(times).

Why add classes at all? A PVSystem class can potentially help to separate the intrinsic and extrinsic parts of a system model. When you make a PVSystem object, you'd give it intrinsic attributes such as lat, lon, and module type. When you want to model the power of your system, you'd need to supply extrinsic data such as irradiance and temperature to the object instance methods. This logical separation seems useful to me, but your mileage may vary.

The proposed class hierarchy is:

Location
PVSystem
SingleAxisTracker

I think there is value in having a Location class because it does not require any system specs to do things like calculate solar position or model clearsky profiles. Maybe it's not worth the extra effort to make this distinction.

Any of these objects could be created by the metadata from a TMY file via the from_tmy classmethod (again, not tested).

[bt-]

@wholmgren I recently discovered pvlib-python and I have been reading through the current conversations in #17 and #84. I am fairly new to the scientific python stack, but I wanted to contribute a few thoughts.

I like the idea of creating a PVSystem class and your logic of extrinsic data and intrinsic attributes makes sense to me.

I think it would be useful to have the ability to run calculations for a location without needing to define a complete PV system. Could this be achieved by creating PVSystem as an Abstract Base Class and having Location as a child class? Or, maybe the PVSystem class is written in a way that it is simple to just create a PVSystem class object with only location attributes?

What type of attributes and methods do you envision for the SingleAxisTracker class?

[wholmgren]

@bt- thanks for the feedback.

We could choose to make the PVSystem constructor accept keyword arguments (probably with None defaults) for most/all attributes. Then it could be as well or as poorly specified as you'd like. We'd need to do something to make it clear that not everything is going to work if you don't fully specify your system. Maybe you just specify lat/lon so that you can use the solar position and clear sky methods. Maybe you just specify a module and inverter so that you can use the power functions. If a PVSystem can be as poorly specified as you'd like, then the usefulness of a separate Location class seems to be pretty low.

What would be the advantage of designating one of these classes as an abstract base class? On first consideration, it to me like an unnecessary complication.

Concerning SingleAxisTracker, I hadn't thought of anything other than what's already in this PR's tracking.py. It has its tracking function and then all of the inherited methods from PVSystem and Location.

[bt-]

@wholmgren I just read the PVLIB Python 2015 paper and I've looked at the code more since my last comment. I think it probably does make sense to keep a Location class and let PVSystem inherit from it.

I'm interested to know what others think about the repository that @dacoex referenced and how much interest there is in general in developing more system modeling capabilities.

[wholmgren]

@bt- I wrote a short book over at that issue in feedinlib, so maybe the conversation will shift there for awhile.

[wholmgren]

Rebased on #98 and fixed/added enough stuff to make get_solarposition and get_clearsky work.

Also added PVSystem.get_irradiance.

I think that it's starting to feel more concrete, but there's still a ton of work to do if we want to make this viable.

Here's a usage example for the stuff that's working. The clearsky data is calculated twice to ensure that it can still call solarposition.get_solarposition. Note that all changes to the object state are explicit. There are no side effects or sneaky assignments from the function calls.

In [1]: import pandas as pd

In [2]: import pvlib

In [3]: tucson_system = pvlib.pvsystem.PVSystem(32, -111, tz='US/Arizona', altitude=700.)

In [4]: times = pd.DatetimeIndex(start='2015-11-09', end='2015-11-10', freq='1h', tz='MST')

In [5]: tucson_system.clearsky = tucson_system.get_clearsky(times, solarposition_method='nrel_numpy')

In [6]: tucson_system.solarposition = tucson_system.get_solarposition(times)

In [7]: tucson_system.clearsky = tucson_system.get_clearsky(times, zenith_data=tucson_system.solarposition.apparent_zenith)

In [8]: tucson_system.solarposition
Out[8]:
                           apparent_elevation  apparent_zenith     azimuth  \
2015-11-09 00:00:00-07:00          -74.679993       164.679993  352.935274
2015-11-09 01:00:00-07:00          -70.735980       160.735980   40.949622
2015-11-09 02:00:00-07:00          -60.420175       150.420175   65.781709
2015-11-09 03:00:00-07:00          -48.290252       138.290252   79.144320
2015-11-09 04:00:00-07:00          -35.660377       125.660377   88.386405
2015-11-09 05:00:00-07:00          -22.966107       112.966107   96.099262
2015-11-09 06:00:00-07:00          -10.445034       100.445034  103.441391
2015-11-09 07:00:00-07:00            1.990856        88.009144  111.142355
2015-11-09 08:00:00-07:00           13.231451        76.768549  119.849257
2015-11-09 09:00:00-07:00           23.611967        66.388033  130.274243
2015-11-09 10:00:00-07:00           32.329110        57.670890  143.193902
2015-11-09 11:00:00-07:00           38.477898        51.522102  159.126926
2015-11-09 12:00:00-07:00           41.057539        48.942461  177.519334
2015-11-09 13:00:00-07:00           39.510573        50.489427  196.246652
2015-11-09 14:00:00-07:00           34.182878        55.817122  212.920693
2015-11-09 15:00:00-07:00           26.025662        63.974338  226.578873
2015-11-09 16:00:00-07:00           15.996566        74.003434  237.557757
2015-11-09 17:00:00-07:00            4.858470        85.141530  246.616923
2015-11-09 18:00:00-07:00           -7.306584        97.306584  254.491218
2015-11-09 19:00:00-07:00          -19.754025       109.754025  261.831776
2015-11-09 20:00:00-07:00          -32.431551       122.431551  269.319049
2015-11-09 21:00:00-07:00          -45.121354       135.121354  277.935232
2015-11-09 22:00:00-07:00          -57.478285       147.478285  289.705774
2015-11-09 23:00:00-07:00          -68.573927       158.573927  310.213534
2015-11-10 00:00:00-07:00          -74.960507       164.960507  352.736388

In [9]: tucson_system.clearsky
Out[9]:
                                 dhi         dni         ghi
2015-11-09 00:00:00-07:00   0.000000    0.000000    0.000000
2015-11-09 01:00:00-07:00   0.000000    0.000000    0.000000
2015-11-09 02:00:00-07:00   0.000000    0.000000    0.000000
2015-11-09 03:00:00-07:00   0.000000    0.000000    0.000000
2015-11-09 04:00:00-07:00   0.000000    0.000000    0.000000
2015-11-09 05:00:00-07:00   0.000000    0.000000    0.000000
2015-11-09 06:00:00-07:00   0.000000    0.000000    0.000000
2015-11-09 07:00:00-07:00  22.533755   82.489482   25.399439
2015-11-09 08:00:00-07:00  52.871820  627.158738  196.419208
2015-11-09 09:00:00-07:00  67.438967  815.735677  394.174074
2015-11-09 10:00:00-07:00  78.293164  892.844936  555.770326
2015-11-09 11:00:00-07:00  85.373563  927.534357  662.497215
2015-11-09 12:00:00-07:00  88.185329  939.022624  704.950989
2015-11-09 13:00:00-07:00  86.511077  932.316849  679.670259
2015-11-09 14:00:00-07:00  80.480921  904.570777  588.701520
2015-11-09 15:00:00-07:00  70.536993  841.629429  439.821821
2015-11-09 16:00:00-07:00  57.053147  694.838880  248.536665
2015-11-09 17:00:00-07:00  34.513402  259.573984   56.497947
2015-11-09 18:00:00-07:00   0.000000    0.000000    0.000000
2015-11-09 19:00:00-07:00   0.000000    0.000000    0.000000
2015-11-09 20:00:00-07:00   0.000000    0.000000    0.000000
2015-11-09 21:00:00-07:00   0.000000    0.000000    0.000000
2015-11-09 22:00:00-07:00   0.000000    0.000000    0.000000
2015-11-09 23:00:00-07:00   0.000000    0.000000    0.000000
2015-11-10 00:00:00-07:00   0.000000    0.000000    0.000000

[dacoex]

I think that it's starting to feel more concrete, but there's still a ton of work to do if we want to make this >viable.

What exactly?

I would remove the get_ prefix.

[wholmgren]

What exactly?

If I make a to do list are you going to help do the work?

I would remove the get_ prefix.

Usually I would agree, however, the get_ prefix allows you do to things like:

tucson_system.clearsky = tucson_system.get_clearsky(times)

Do you have an alternative suggestion?

[dacoex]

If I make a to do list are you going to help do the work?

Yes, but very step by step because my main work is not about modelling and code development.

I am currently interest in getting pvlib toward the status to have something like what was in the tutorial operational:
#88

Do you have an alternative suggestion?

Not now. One would just have to check that the get_ functions get too long.

[wholmgren]

I am currently interest in getting pvlib toward the status to have something like what was in the tutorial operational: #88

Go for it. I am looking forward to the pull request.

[dacoex]

Not now. One would just have to check that the get_ functions get too long.

Not this one. The only one with this issue would potentially be get_poa_horizontal_ratio. But it's I minor issue. I think we should implement things. Most discussion has come to a good conclusion. The rest coudl be sorted on the way.

[dacoex]

@wholmgren as follow-up to #93 (comment) I created a page for Roadmap.

Hope this helps and does not add too much overhead.

[uvchik]

According to @wholmgren's post at feedinlib I will give some feedback.

I really like the idea of the ModelChain class. It lowers the entry barrier for new users and maybe we can shorten our feedinlib-pv-code by using this class.

So the modelchain is great but what is the idea to use oop instead of procedural programming? What is the real effort from In [16] to In[33] at http://wholmgren-pvlib-python-new.readthedocs.org/en/pvsystem/package_overview.html#modeling-paradigms

As I understand it one advantage of oop is that you can inherit. So we could create a new class PVSite (name is not important) which inherits the PVSystem class but has some more attributes such as location, weather(optional),.... This class could have a method run_model and an attribute ac. The code would look as follows:

my_pv = PVSite(module_parameters=my_mod, inverter_parameters=my_inv, location=my_loc)
my_pv.run_model(times)
annual_ac_energy = my_pv.ac.sum()
annual_dc_energy = my_pv.dc.sum()

I know that this is a matter of taste but I prefer attributes instead of using the methods like functions. If one method calculates aoi than the other method should use self.aoi. I thought the new thing of oop is that the object contains inputs and outputs. If there are too many attributes you can still inherit or split classes.

These are just ideas. Thank you for your approach.

[wholmgren]

Thanks @uvchik for your helpful comments.

So the modelchain is great but what is the idea to use oop instead of procedural programming?

I'm not sure if you only meant this as a lead-in to your next paragraph or if you wanted an answer, but... I was thinking about this too and I don't have a good answer. I mostly implemented it this way because that's what other people wanted to see. I do think that a basic model chain function should be added to this PR.

I know that this is a matter of taste but I prefer attributes instead of using the methods like functions. If one method calculates aoi than the other method should use self.aoi. I thought the new thing of oop is that the object contains inputs and outputs.

There have been a lot of angry words spewed on this topic on the internet. I used to use a lot of side effects and no return values. My code inevitably turned into a nasty mess. Of course, it's possible to write bad code using any design pattern.

I suppose that there's something to be said for the idea that if we're going to make both frameworks then the OO framework should be as high-level as possible so that it's most clearly separated from the functional framework.

As a compromise of sorts, the SingleAxisTracker.get_irradiance method currently looks for a couple attributes if they're not provided as kwargs. The idea here was that the system's attributes (surface_tilt and surface_azimuth) are probably linked to the solar position, so it might make sense to assign all of these attributes to the system during your modeling. I don't know if this is a good idea or not.

[alorenzo175]

I've only briefly looked over the changes, but I'm happy with what I've seen. I think ModelChain probably still needs some work that can be deferred to the future like tracking the exact modeling steps and exporting to a file.

I mostly agree with @uvchik that ModelChain should set attributes in addition to returning the output. This reduces the variables one has to keep track of and it makes sense for how it will likely be used.

I also like the idea of a tracker base class.

[uvchik]

I'm far away from being an expert in OOP. As I understand it one should divide the full System into its components. For a table you would have a DeskTop class, a TableLeg class and a Table class. Than you would create four TableLeg instances and a DeskTop instance and use them to create a Table instance. For the PV-System this is more abstract because you have weather conditions and locations which are also parts of your PVSimulation object. Furthermore some python "rules" say that a class should not have more than seven attributes and x methods. Otherwise you have to define smaller classes. That's how I understand it. The question is how we want to use it and what are the requirement of the users.

I would say that the most important thing is that we have a nice high level class that works and I see that ModelChain is already a useful class and for version 0.3 I would only do little changes.

@wholmgren, just decide what suggestions you want to implement.

• A ModelChain object for me is less speaking than a PVSimulation object or something similar. I would rename it.
• I still would inherit from the PV-System class (or from a Tracker class) as I explained in my last comment instead of adding the PVsystem as an attribute.
• Having al least ac and dc as attributes.

If we have classes with some attributes I could add a dump and a restore method to save and load the attributes of an object.

[wholmgren]

Thanks for the comments and sorry for the delay in getting back to this.

@uvchik, regarding your name and inheritance preferences... I'm going to go with what we have now but I am open to changing all of this in the future. After this is merged, please make a new issue(s) for these ideas if you'd like to continue to discuss them. I think some of this will change a lot in the future.

Some changes:

• Added a basic_chain function to the modelchain module.
• Added a basic_chain example to the Package Overview. Unfortunately, RTD is currently refusing to build documentation due to an error on their end.
• The ModelChain.run_model method now assigns all intermediate calculations as attributes of the mc object, while returning the tuple (dc, ac). I would be open to returning None, but let's leave that for future discussions.
• Removed the unimplemented model chain code from early examples.
• Cleaned up modelchain documentation.
• Added a few tests (100% modelchain coverage, 100% location coverage, 93% pvsystem coverage (the omissions are not testable and/or very minor).
• Pinned the scipy version in the Appveyor builds to avoid a problem with Continuum's scipy build for Windows.

I'm going to merge this on Tuesday unless I hear objections (this time I really mean it).

[uvchik]

@wholmgren : Good work. Thank you. Yes I really think you should merge it, because it is a very helpful feature.

By the way. I have been told to never return an attribute. An alternative way is to return the object itself to allow chaining.

    self.dc = ....
    self.ac = ...
    return self

# Use the class:
mc = ModelChain(system, location, ...)
annual_energy = mc.run_model(times).ac.sum()

Just an idea for future development. No need to open an issue for me. I think we better wait a while and let people use and test this feature. Based on this experiences we can open new issues.

[wholmgren]

Returning self is an excellent idea. I updated the code accordingly.

Also, the new documentation is working again.

[wholmgren]

Thanks to all for your ideas and help in developing this massive PR. I'm sure that it will take more work to get it right, so please install the new master and try it out. See you on the issue tracker...

[dacoex]

Thanks to @wholmgren for being the driver and actaully implement it.
I think it offers huge potential in terms of workflow and simplification.
Thanks again for the great work here.

[uvchik]

👍 Great feature, thank you @wholmgren .