-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
meta, location, and systemdef philosophy #17
Comments
These are all good points/questions. This is definitely a place where we need to improve the package. I propose that we:
For |
Great, that is what I was thinking, but I wasn't sure there was something I was missing. Both the 0.1 and 0.2 solutions make sense to me. I am interested in helping on small fixes like this, but as I'm an advanced beginner in python, and a beginner in github, is there a good reference doc for the process preferred by this repo of making some of these changes and presenting them for review? |
@jforbess would you like to make a pull request with the 0.1 fixes? Should probably add a |
@wholmgren sure, I'll do that, but not until Sunday or Monday. |
@bmu and @Calama-Consulting do you support the proposal above or do you have other ideas for how to resolve this? I think is a high priority for 0.2 since it could impact the API in fairly significant ways. @jforbess, sorry, just now realizing that I did not respond to your question regarding contributing. Does the Development information wiki help? |
As mentioned by @jforbess I think that one often uses a lat/lon different from the TMY data. So I am not really sure, if it is a good idea to return the A
We cannot cover all of this in pvlib (maybe this would be a good topic for a PhD and I think it would be very useful to have a standard for the whole PV community). And up to now we don't need all of this (e.g. we don't have models for calculation of shading losses, wiring losses, transformers ...). But maybe we should have it on mind when we implement something like this. And I agree, this is something important for our API, so we should have something quite stable for the future. |
My comment in #39 is possibly related (is it possible to link to a specific comment?) |
Thanks, those comments are a nice place to start the discussion. It was pretty natural for me to think about Another thing to keep in mind is that there might be situations where some models can only be used with certain kinds of systems. At the risk of getting off topic... I'm not sure why it matters that you may want to use the TMY data for a nearby but different location. In going from TMY to I'm not really sure where to start here. Should we mock up some objects and example code as in @bmu's comment? (Click on the date of the comment to get a link to it.) |
Thanks to @jforbess for creating a new tutorial in #88 and making me think about this again. In the spirit of retaining the low-level, mostly non-object-oriented nature of pvlib, I think that we should could expand the definition of
I now think that methods such as site = pvlib.location.Location(lat, lon, tz, elev, name)
solpos = site.get_solarposition(times) or site = {'lat':lat, 'lon':lon, 'tz':tz, 'elev':elev, 'name':name}
solpos = pvlib.solarposition.get_solarposition(**site) To make the object oriented stuff work using the procedural code we can do things like class Location(object):
# stuff like init
def get_solarposition(self, times):
return pvlib.solarposition.get_solarposition(times, self.lat, self.lon)
# maybe other wrapper stuff for other module functions
class PVSystem(Location):
def __init__(module_type, other_system_crap, args_and_kwargs_for_Location):
self.module_type = module_type
# and so on for the rest of other_system_crap
# maybe do something to make non-Location systems (i.e. indoor testing) reasonable
super().__init__()
def calcparams_desoto(self, poa_globa, temp_cell):
return pvsystem.calcparams_desoto(poa_global, temp_cell, module_parameters=self.module_type)
def singlediode(self, params)
return pvsystem.singlediode(attributes, params)
# remember that get_solarposition was inherited from Location, too I worry about the maintenance cost for this, but maybe it would be worth it. @bmu objected to this inheritance, so that needs to be readdressed too. I also found this post by Matthew Rocklin to be helpful in thinking about dictionaries vs. classes. |
I forgot that Current: def ineichen(time, location, linke_turbidity=None,
solarposition_method='pyephem', zenith_data=None,
airmass_model='young1994', airmass_data=None,
interp_turbidity=True): Proposed: class Location(object):
#other stuff here as described above
def ineichen(time, **kwargs):
return clearsky.ineichen(time, self.latitude, self.longitude, **kwargs)
def ineichen(time, latitude, longitude, linke_turbidity=None,
solarposition_method='pyephem', zenith_data=None,
airmass_model='young1994', airmass_data=None,
interp_turbidity=True): |
I meant to respond to your earlier thoughts about this, but time slipped I definitely agree that Location doesn't seem to be an especially useful |
It's been long enough now that I've had to remind myself of why things are the way they are. I figured that I might as well put it the results here... The Matlab toolbox uses location structs in a few places, e.g.: https://github.com/Sandia-Labs/PVLIB_Matlab/blob/master/PVLIB/pvl_makelocationstruct.m https://github.com/Sandia-Labs/PVLIB_Matlab/blob/master/PVLIB/pvl_clearsky_ineichen.m https://github.com/Sandia-Labs/PVLIB_Matlab/blob/master/PVLIB/pvl_ephemeris.m Rob's original Python port copied most of this behavior: https://github.com/Sandia-Labs/PVLIB_Python/blob/master/pvlib/pvl_makelocationstruct.py https://github.com/Sandia-Labs/PVLIB_Python/blob/master/pvlib/pvl_clearsky_ineichen.py https://github.com/Sandia-Labs/PVLIB_Python/blob/master/pvlib/pvl_ephemeris.py One place where the Matlab code and the original Python code diverged was in the TMY reader results, also known as "meta". The Matlab version put data and metadata into the same struct, where as the Python version had to break out the metadata into a dict so that the data could be in a DataFrame. I wanted PVLIB_Python to use the patterns and terminology of a standard Python library, so I created a PR (Sandia-Labs/PVLIB_Python#17) that replaced the location struct with a In retrospect, I believe that was a missed opportunity to drop the I created a prototype PR #93 to help make the discussion about the future more concrete. |
Commenting on #17 (comment) after having gone through #93 (comment) It seems that
Is this correct? What would be the benefit? Would it allow me to have systems that inherit some parameters (tilt angle) while changing others (module type)? |
I don't know what sth is. |
something |
Ah. No I would not. In the PV_LIB Matlab context the Location structure served only to pass three arguments (lat, long and altitude) with a single variable. The shortcut didn't seem worth the bother of having a structure. When there's many variables that have to stay together (like coefficients for a PV module model, or a measured IV curve) then a structure made sense. |
I think that we may actually be in agreement so let me try to clarify. The half-baked code that I'm showing in #93 introduces a new |
I have always been confused as to the value of Location as a special object So I am currently on board with Will, I think. |
If I understand this correctly, you are now thinking that data that are intrinsic to a location (latitude, longitude, weather) will be subordinate to a PVsystem? I think the two (PVsystem and Location) would be better separated. What if one was trying to optimize a design for a particular site, and wanted to “loop” over many PV systems? But what I know about object-oriented code can be summarized in less text than this email. |
I frequently run parts of the PV modeling sequence, including clear sky models, for purposes other than calculating power. For example, to investigate the quality of satellite irradiance data. I understand jforbess’ point, just a thought that there are broader uses for this code. |
Agree, also loop the same system through different sites. |
These are good points. I don't know if there is a right answer to these problems. Some more half-baked ideas to consider:
The diversity of opinions and use cases reinforces the need to keep the procedural code clean and flexible. Whatever object oriented API we ultimately go with will just be one possible solution. |
Here's one way to loop over locations using the third idea in my previous comment. base_system = PVSystem(module, inverter, **other_params)
coordinates = [(30, -110), (35, -100), (40, -120)]
times = pd.DatetimeIndex(start='2015-01-01', end='2016-01-01', freq='1h')
energies = []
for latitude, longitude in coordinates:
localized_system = base_system.localize(latitude, longitude)
cs = localized_system.get_clearsky(times)
solpos = localized_system.get_solarposition(times)
total_irrad = localized_system.get_irradiance(times, **solpos, **cs)
power = localized_system.get_power(stuff)
annual_energy = power.sum()
energies.append(annual_energy) |
From my point of view it is still the best idea to have a separated The The So one could write: coordinates = [(30, -110), (35, -100), (40, -120)]
pv_system = PVSystem(module, inverter, **other_params)
times = pd.DatetimeIndex(start='2015-01-01', end='2016-01-01', freq='1h')
results = {}
for latitude, longitude in coordinates:
location = Location(latitude, longitude)
location.get_data(source='TMY')
# or alternatively: source='some_file.csv', type='Meteonorm7' or 'SolarGIS' ...
# alternatively:
# location.get_clearsky(times)
# location.get_solarposition(times)
# location.get_irradiance(times, **solpos, **cs)
pv_system.set_location(location)
pv_system.calculate_power()
# store all data from all modelling steps
results[(latitude, longitude)] = pv_system.data
# or store only summaries
energy = pv_system.data['AC_power'].sum()
gpoa = pv_system.data['GPOA'].sum()
pr = energy / gpoa * 100.0
results[(latitude, longitude)] = {'PR': pr, 'GPOA': gpoa, 'Energy': energy} Same could be done when you want to cycle with different systems for a single location. Also, maybe it would be useful to have another class from pvlib.modelling_chains import DefaultModelingChain
pv_system = PVSystem(module, inverter, **other_params)
model_chain = DefaultModelingChain(conversion_model='Klucher')
pv_system.model_chain = model_chain |
I agree
This would have made the Matlab Location structure useful.
I absolutely support documenting choice of models and parameters, but I'd tie that information to the calculation of energy yield. I'm not sure that creating a new class to contain inputs helps. If a new class is the way to go, I'd broaden that class so that an instance contains both the inputs and outputs from the energy simulation. |
The idea of the ModelChain class is more about flexibility in modeling. could also be a function, but a class allows to change single modeling steps without the need to write a whole new function. I think documenting the results is a different topic. At my institute we use to store the results and all parameters used to get these results in a single plain text file. This should be easy to implement for pvlib, too. Could be a method of |
I would second the proposal put forward in #17 (comment) especially the Let's get this started. |
@jforbess: have @bmu and @cwhanse changed your mind? @bmu's example brings up another design issue: side effects. Most of @bmu's example code mutates the state of the Against side effects:
For side effects:
|
Regarding
@dacoex This is a little frustrating to hear. What do you think #93 is if not an attempt to get this discussion moving in a more concrete direction? |
I've been hoping to have more time to explore the library before commenting, but here are some poorly organized thoughts.
I like this approach. I'm not sure I agree totally with @bmu and @cwhanse that a I will put in my vote for avoiding side effects. I have minimal experience with development, but it makes sense to me to stick with the conventions of the other libraries in the PyData stack.
I'm not sure I fully understand what would differentiate the I think it would make sense to save out results data to a csv using pandas. This is the format used by other PV modeling tools. I agree with the idea of storing inputs and outputs together as a matter of practice, but I think in my own use I would prefer to save them in separate files located in the same directory. |
Sorry, that was not the intention here.
Yes, sure.
Same would apply for me case. At least when it comes to structuring the re-fractoring of a whole library.
This is good and that's why this discussion is useful. That done, all will be clear.
There are other calculations not directly related to PV system: data quality checks, extended statistics and plotting, etc. |
Here are some thoughts on how to move forward. First, I am strongly opposed to any methods that cause side effects. Debugging or changing code that implicitly modifies objects is frustrating and time consuming. I also don't see a need for Then, a class ModelChain(object):
def __init__(self, system, location, solar_position_method, clearsky_method, times,...):
self.system = system
...
def run_model(self):
solar_position = self.location.get_solarposition(self.times)
clearsky = self.system.get_clearsky(solar_position, self.clearsky_method)
...
final_output = self.system.calculate_final_yield(args)
return final_output
def model_system(self):
final_output = self.run_model()
input = self.prettify_input()
modeling_steps = self.get_modeling_steps()
# print/write out the input, modeling steps, and output to a file or in some format It is pretty simple to just define a system, a location, and the models you want to use and do mc = ModelChain(system, location, otherstuff)
output = mc.run_model() Maybe we could add a convenience function to save everything or the user can do some |
This sounds good! Better this way, that the ModelChain takes system and location as arguments. |
Tony's proposals for moving forward make sense to me. The structure and classes seem intuitive. |
Thanks everyone for your thoughts on this issue. I'm now seeing almost unanimous support for independent @jforbess: my understanding is that you would have favored a single class that combines it all. It lieu of that we can also experiment with a class PVSystem(object):
def __init__(self, module, inverter, **kwargs):
print('initializing a PVSystem')
self.module = module
self.inverter = inverter
super(PVSystem, self).__init__(**kwargs)
class Location(object):
def __init__(self, latitude, longitude, **kwargs):
print('initializing a Location')
self.latitude = latitude
self.longitude = longitude
super(Location, self).__init__(**kwargs)
# inheritance order should not matter if defined as above
class LocalizedPVSystem(PVSystem, Location):
def __init__(self, **kwargs):
print('initializing a LocalizedPVSystem')
super(LocalizedPVSystem, self).__init__(**kwargs)
print('LocalizedPVSystem initialized')
In [17]: localized_pv_system = LocalizedPVSystem(latitude=30, longitude=-110, module='blah', inverter='blarg')
initializing a LocalizedPVSystem
initializing a PVSystem
initializing a Location
LocalizedPVSystem initialized
In [18]: localized_pv_system.latitude
Out[18]: 30
In [19]: localized_pv_system.module
Out[19]: 'blah'
In [21]: pv_system = PVSystem(module='blah', inverter='blarg')
initializing a PVSystem
In [22]: pv_system.module
Out[22]: 'blah'
In [23]: location = Location(latitude=30, longitude=-110)
initializing a Location
In [24]: location.latitude
Out[24]: 30 I do think that some people would find this to be a more useful paradigm than a combination of |
Just two thoughts, but mayby these are to advanced things at least for the moment: First, for e.g. East West orientated modules and strings that are connected to a single Inverter / MPP tracker one would need Modeling chains that start with horizontal irradiance and model until DC. Than the results of these model chains must be combined and feed into a modeling chain that starts from the mpp tracker or inverter and model everything until final energy yield. So one would need ModelingChains that model not every step until final yield and the ModelingChain should be able to take results from other chains as arguments and combine them. Second I was trying to integrate uncertainty calculation into our simulation and I endet up with classes that encapsulate every modeling step. These classes take a description oft their uncertainties and they are stacked into a modeling chain. So the chain has access to all uncertainties of their steps and can manage the propagation of uncertainties until final energy yield. |
I would love to see both of these sets of functionality developed fully. I see these potential features as areas where PVsyst and SAM fall somewhat short. I think SAM does have some decent capabilities for uncertainty analysis, but not the ability to propagate uncertainty through the model from inputs to final energy yield. |
I am going to close this issue now #93 has been merged. I'm sure that we will need some new issues to discuss the modeling philosophy, the API and the implementation of the new features. |
I don't fully understand the differences of meta, location, and systemdef, in terms of how they should be used.
systemdef expects TMY meta to be used in its first parameter. But what if I want to use the lat/long for my plant, not the TMY station? What if I am looking at operating data (which I am), and have no need to pull in a TMY file?
Why doesn't systemdef use a location instead of a TMY meta as its first parameter? (Why isn't a TMY meta a Location object?)
The text was updated successfully, but these errors were encountered: