Support list of weather input for ModelChain with a single-array PVSystem #1157

wfvining · 2021-02-02T17:17:56Z

Closes ModelChainResult.cell_temperature is not always a pandas.Series #1139
I am familiar with the contributing guidelines
Tests added
~~Updates entries to docs/sphinx/source/api.rst for API changes.~~
Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for 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`).
New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
Pull request is nearly complete and ready for detailed review.
Maintainer: Appropriate GitHub Labels and Milestone are assigned to the Pull Request and linked Issue.

Adds a layer of indirection for assigning attributes of ModelChain.results that ensures the types of the results are consistent with the type of the input (i.e. ModelChain.weather).

Covers passing a length-one list of weather DataFrames to each of - `ModelChain.run_model()` - `ModelChain.run_model_from_poa()` - `ModelChain.run_model_from_effective_irradiance()` When the PVSystem being modeled has only one Array. In this case the output stored in each field of `ModelChain.results` should be a length-one tuple.

Allow p_dc and v_dc to be length-1 tuples even when the system has only one Array. This helps keep the API consistent so that if the dc power methods are called with `unwrap=False` their output can be passed directly to `PVSystem.get_ac()`.

This method is responsible for ensuring that per-array results match the type of ModelChain.weather. This is an issue when ModelChain.system has only one Array, but ModelChain.weather is a tuple of length 1. In this case we want the results attributes to also be length-1 tuples for the sake of consistency and to ensure that calculations down-stream can proceed without needing to worry about mixed types (e.g. total_irrad is a Series, but weather is a tuple).

Accidentally under-indented a whole test: test_run_model_from_poa_singleton_weather_single_array()

wholmgren · 2021-02-02T17:47:38Z

At the risk of making things more complicated... what do you think about using a setter instead of explicitly adding the private function call to many of the assignment lines? I'm undecided.

cwhanse · 2021-02-02T17:54:48Z

pvlib/pvsystem.py

            if multiple_arrays:
-                p_dc = self._validate_per_array(p_dc)


why move the _validate_per_array outside if multiple_arrays? IIRC, that doesn't work when p_dc and v_dc are floats.

If p_dc and v_dc are floats they will get wrapped up as (p_dc,) and (v_dc,). Doing this regardless of the number of arrays lets us always subscript when passing to the single array inverter models. If we didn't wrap them up we would need extra logic to handle unwrapping if the input was initially a singleton tuple.

The goal is to be able to do both of these and have them act the same:

system = PVSystem(arrays=[Array()], ...) system.get_ac(p_dc=50, model='pvwatts') system.get_ac(p_dc=(50,), model='pvwatts')

As written, get_ac references the inverter functions directly, not through a PVSystem method for each inverter function. The inverter functions don't know what to do with a tuple of values; they assume numeric.

The tuples are always subscripted before being passed to the inverter functions when multiple_arrays is False. For example:

if model == 'sandia': p_dc = self._validate_per_array(p_dc) v_dc = self._validate_per_array(v_dc) if multiple_arrays: return inverter.sandia_multi( v_dc, p_dc, self.inverter_parameters) return inverter.sandia(v_dc[0], p_dc[0], self.inverter_parameters)

When multiple_arrays is True the tuples are passed directly to inverter.sandia_multi, but p_dc[0] and v_dc[0] is used for inverter.sandia.

I think we can now move the _validate_per_array calls above the if model ==... block and save a few lines:

p_dc = self._validate_per_array(p_dc) if v_dc is not None: v_dc = self._validate_per_array(v_dc)

wfvining · 2021-02-02T20:13:54Z

At the risk of making things more complicated... what do you think about using a setter instead of explicitly adding the private function call to many of the assignment lines? I'm undecided.

The problem with a setter is that it is setting fields on ModelChainResult which doesn't know what the right type should be. That information is only accessible in ModelChain.weather. Maybe that can be addressed by something like self.results._tuple_results(self.system.num_arrays > 1 or isinstance(self.weather, tuple))
Moving the coercion inside ModelChain would have the benefit of hiding the distinction between per-array and system-wide results. Currently that distinction is ad-hoc and handled by either using or not using _assign_result()

cwhanse · 2021-02-02T20:30:14Z

self.results._tuple_results(self.system.num_arrays > 1 or isinstance(self.weather, tuple))

Related note: should we infer the type of a result value from the type of weather, or from system.num_arrays value? Current code may be using both techniques in different places.

wfvining · 2021-02-02T21:13:11Z

Related note: should we infer the type of a result value from the type of weather, or from system.num_arrays value? Current code may be using both techniques in different places.

It needs to be based on weather to handle the run_model([weather]) case correctly. There are a good number of places where we use num_arrays, but it is typically when we are manufacturing data (e.g. setting the loss for each array to 1.0) or expanding non-tuple weather to be a tuple. I think these cases are okay, everywhere we are concerned with type we are using isinstance or _assign_result.

cwhanse · 2021-02-02T21:20:54Z

Related note: should we infer the type of a result value from the type of weather, or from system.num_arrays value? Current code may be using both techniques in different places.

It needs to be based on weather to handle the run_model([weather]) case correctly. There are a good number of places where we use num_arrays, but it is typically when we are manufacturing data (e.g. setting the loss for each array to 1.0) or expanding non-tuple weather to be a tuple. I think these cases are okay, everywhere we are concerned with type we are using isinstance or _assign_result.

run_model([weather]) with a 2-array PVSystem produces modelchain.result.total_irrad = [(df1, df2)], a list with a single element which is a two-tuple? Asking so I better understand the intent.

wfvining · 2021-02-02T21:23:07Z

run_model([weather]) with a two array system raises a ValueError. If you pass weather as a list or tuple it must be the same length as system.arrays.

wfvining · 2021-02-02T21:36:31Z

Oops... forgot to address a couple TODOs. Adding tests with spectral_loss != 'no_loss' and aoi_loss != 'no_loss'

wfvining · 2021-02-02T22:00:56Z

Here is an interesting use case. If I want to set constant spectral loss as in this test, but I am calling run_model([weather]) then ModelChain.results.spectral_modifier will have the wrong type.

def constant_spectral_loss(mc):
    mc.results.spectral_modifier = 0.9


@pytest.mark.parametrize('spectral_model', [
    'sapm', 'first_solar', 'no_loss', constant_spectral_loss
])
def test_spectral_models(sapm_dc_snl_ac_system, location, spectral_model,
                         weather):
    # add pw to weather dataframe
    weather['precipitable_water'] = [0.3, 0.5]
    mc = ModelChain(sapm_dc_snl_ac_system, location, dc_model='sapm',
                    aoi_model='no_loss', spectral_model=spectral_model)
    spectral_modifier = mc.run_model(weather).results.spectral_modifier
    assert isinstance(spectral_modifier, (pd.Series, float, int))

Since execution of constant_spectral_loss is deferred (+1 for whoever made that a thunk) until after we know the right types this could be handled by the setter approach discussed above.

Things might get weird when there are multiple arrays though; since there currently is not mechanism to turn the constant in to a tuple as we do for no_loss, users need to anticipate this and set the correct type. Not totally unreasonable since they have access to self.system.

Add tests for spectral_model != 'no_loss' and aoi_model != 'no_loss' when calling ModelChain.run_model([weather]) on a system with 1 Array. Handles the type of ModelChain.weather correctly in ModelChain.first_solar_spectral_loss() by using _tuple_from_dfs() to get 'precipitable_water'. Still needs support for constant spectral loss

Extends `PVSystem.first_solar_spectral_loss()` to treat `pw` as a per-Array or system wide parameter.

A bit cleaner than using a generator expression.

Add a configuration step in ModelChain._assign_weather() that sets a flag on the ModelChain.results to indicate whether non-tuples assigned to per-array fields should be wrapped in length-1 tuples. This is accomplished by a custom __setattr__ methon on ModelChainResult. The setter checks whether the attribute is a per-array attribute and coerces it if the _singleton_tuples flag is set.

Covers the case where the aoi_model is 'no_loss'.

wfvining · 2021-02-03T16:30:13Z

@wholmgren I have refactored to use a setter-based approach. Take a look and let me know what you think. I believe it is an improvement.

wholmgren

@wfvining I like the approach. A couple of comments below - I've not looked carefully at the pvsystem changes or tests.

wholmgren · 2021-02-03T20:33:45Z

pvlib/modelchain.py

+        return value
+
+    def __setattr__(self, key, value):
+        if key in ModelChainResult._per_array_fields:


could we use typing.get_type_hints and delete the private set?

That would be nice, but I can't seem to figure out how to compare the concrete types returned from get_type_hints with the generic Optional[PerArray[_T]] type.

After further research, my idea seems like a misuse of the typing module at this point in time, so I'm good with your solution.

I'm a little bummed... I was excited to actually put the types to use.

wholmgren · 2021-02-03T20:40:46Z

pvlib/modelchain.py

@@ -1271,6 +1291,17 @@ def _verify(data, index=None):
            for (i, array_data) in enumerate(data):
                _verify(array_data, i)

+    def _configure_results(self):


an alternative to this method might be to instantiate ModelChain with self.results = None (or not at all), then put this line at the start of each run_model workflow:

self.results = ModelChainResults(singleton_tuples=(self.system.num_arrays == 1 and isinstance(self.weather, tuple))

I guess maybe you'd still want to extract that logic into a method like this one. Then the difference comes down to setting the right value vs instantiating the object with the right value. 🤷

This would basically be a transformation from _configure_results to _prepare_results. I kind of like that better, but I don't think it makes a big difference. It would also take a little bit of work, since some of the tests (and ModelChain methods) make the (reasonable) assumption that all the results fields exist before one of the run_model workflows is triggered (e.g. test__assign_total_irrad and ModelChain.complete_irradiance).

Should _configure_results() (or some ModelChainResult method) iterate over per-array fields to and wrap them in tuple if _singleton_tuples is changed from False to True?

I'm viewing ModelChainResult as a single use object so I don't think we need to watch for that kind of state change.

pvlib/modelchain.py

wholmgren · 2021-02-04T04:25:29Z

pvlib/pvsystem.py

            if multiple_arrays:
-                p_dc = self._validate_per_array(p_dc)


I think we can now move the _validate_per_array calls above the if model ==... block and save a few lines:

p_dc = self._validate_per_array(p_dc) if v_dc is not None: v_dc = self._validate_per_array(v_dc)

wholmgren · 2021-02-04T04:43:23Z

pvlib/tests/test_pvsystem.py

+        arrays=[pvsystem.Array()],
+        inverter_parameters=inverter_parameters[model]
+    )
+    ac = system.get_ac(p_dc=(pdcs[model],), v_dc=(vdcs[model],), model=model)


Do we actually want to support v_dc=(None,) for model='pvwatts'? If we do support that, then my suggestion to move the _validate_per_array calls won't work.

I think I started with these calls above the control flow, but ran in to this. I don't really like them inside the control flow, but I don't see a better way.

Re-order fields and comments. Add docstring to _result_type() method.

wholmgren

thanks @wfvining. I also confirmed that the example in #1139 no longer fails.

wfvining added 4 commits February 1, 2021 14:57

Clean up whitespace in test_modelchain.py

42f7ca0

Accidentally under-indented a whole test: test_run_model_from_poa_singleton_weather_single_array()

cwhanse added the bug label Feb 2, 2021

cwhanse added this to the 0.9.0 milestone Feb 2, 2021

cwhanse reviewed Feb 2, 2021

View reviewed changes

wfvining added 5 commits February 2, 2021 15:28

PVSystem.first_solar_spectral_loss() validates pw param against

4461f45

Extends `PVSystem.first_solar_spectral_loss()` to treat `pw` as a per-Array or system wide parameter.

Use itertools.starmap in in PVSystem.first_solar_spectral_loss()

6ab9bab

A bit cleaner than using a generator expression.

Add float to the type for ModelChainResult.aoi_modifier

562d729

Covers the case where the aoi_model is 'no_loss'.

alorenzo175 mentioned this pull request Feb 3, 2021

update _modelchain_ac_model for multi array inverters SolarPerformanceInsight/solarperformanceinsight#109

Closed

wholmgren reviewed Feb 3, 2021

View reviewed changes

wholmgren reviewed Feb 4, 2021

View reviewed changes

Tidy up ModelChainResult

081c258

Re-order fields and comments. Add docstring to _result_type() method.

wholmgren approved these changes Feb 4, 2021

View reviewed changes

wholmgren merged commit 47654a0 into pvlib:master Feb 4, 2021

Support list of weather input for ModelChain with a single-array PVSystem #1157

Support list of weather input for ModelChain with a single-array PVSystem #1157

Uh oh!

Conversation

wfvining commented Feb 2, 2021 • edited by cwhanse Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wholmgren commented Feb 2, 2021

Uh oh!

cwhanse Feb 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wfvining commented Feb 2, 2021

Uh oh!

cwhanse commented Feb 2, 2021

Uh oh!

wfvining commented Feb 2, 2021

Uh oh!

cwhanse commented Feb 2, 2021

Uh oh!

wfvining commented Feb 2, 2021

Uh oh!

wfvining commented Feb 2, 2021

Uh oh!

wfvining commented Feb 2, 2021

Uh oh!

wfvining commented Feb 3, 2021

Uh oh!

wholmgren left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wholmgren Feb 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wfvining Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wholmgren left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wfvining commented Feb 2, 2021 •

edited by cwhanse

Loading

cwhanse Feb 2, 2021 •

edited

Loading

wholmgren Feb 4, 2021 •

edited

Loading

wfvining Feb 3, 2021 •

edited

Loading