DC Ohmic Losses for pvsystem and modelchain #1168

ncroft-b4 · 2021-02-15T17:36:52Z

partially addresses Applied PVsyst-type losses in ModelChain #1069
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 functionality for array level losses and adds functions for dc ohmic losses to pvsystem and modelchain. Replaces #1084 for compatibility with Array update.

pvlib/pvsystem.py

cwhanse · 2021-02-23T20:39:03Z

pvlib/pvsystem.py

+        kwargs = _build_kwargs(['dc_ohmic_percent'],
+                               self.array_losses_parameters)
+
+        kwargs.update(_build_kwargs(['V_mp_ref', 'I_mp_ref'],


I wonder if there is a way to extend this to the other DC module models. If module_parameters is taken from the CEC database, it will include V_mp_ref and I_mp_ref.

The other cases are Pvsyst, SAPM and PVWatts.

For Pvsyst, pvlib provides no parameter database. The pvsyst DC power functions don't require V_mp_ref or I_mp_ref, so the user would need to know that these must be added.

For SAPM, the expected names are Vmpo and Impo.

For PVWatts, the ohmic losses model can't be applied since PVWatts only deals with power (not voltage and current separately).

V_mp_ref, I_mp_ref and dc_ohmic_percent are required by the downstream method. Maybe raise a ValueError here if any are missing at this step.

In the ModelChain dc_ohms_from_percent method, we could check dc_model and raise a more informative message how to correct the ValueError.

Ok. I need to sort out the relations between the DC models and the module parameters, which have always been a bit confusing, I admit. I would like to make this applicable for CEC, SAPM, and PVsyst models. Sounds like for SAPM, I can add the option to use Vmpo and Impo if those are present in the module parameters. But for PVsyst, it may require a different workflow, or like you say, to raise a ValueError to say that those parameters need to be added. I will look into this to add an update.

Do you know if the Pvsyst software names these two quantities? If so, I'd use those names. A user will have to add them to module_parameters regardless since there's no data source from which to inherit the names.

I added a section that checks from and adds the parameters in sets of either V_mp_ref, I_mp_ref; Vmpo, Impo; or Vmpp, Impp` (pvsyst-like, from the documentation and pan file parameters). It should raise a value error if these are not found. If this looks like a workable solution, should more tests be added for each case?

kandersolar

A few quick comments about docstring formatting

pvlib/pvsystem.py

ncroft-b4 · 2021-02-24T17:55:50Z

pvlib/modelchain.py

+        of the PVsystem.
+        """
+        Rw = self.system.dc_ohms_from_percent()
+        self.dc_ohmic_losses = pvsystem.dc_ohmic_losses(Rw, self.dc['i_mp'])


I just realized this might need to be rewritten to handle the case where there are multiple arrays and so dc_ohms_from_percent returns a tuple instead of a single value. In that case what will self.dc[i_mp] be? Commenting to mark to investigate and fix.

self.results.dc will be a tuple of DataFrame.

Thanks. I added functionality for multiple arrays in the latest commit. I think all of the comments are addressed now and it is ready for the review and push forward.

wholmgren

Thanks @ncroft-b4. Minor items below. Nice job with the tests.

wholmgren · 2021-03-03T17:16:26Z

pvlib/modelchain.py

+        """
+        Rw = self.system.dc_ohms_from_percent()
+        if isinstance(self.results.dc, tuple):
+            self.dc_ohmic_losses = tuple(


Do we want to assign this to self.results.dc_ohmic_losses? If yes, we'll need to add it to the ModelChainResults class. If not, we should drop self. and leave it as a variable that's just used within this function.

I think it should be assigned to the results. Otherwise there is no record of what power was taken out of p_mp and no way to confirm, test, or just review the losses after the fact.

It looks like pvwatts_losses still creates a self.losses attribute. Do we want to think about coordinating the storage of those losses with other losses like dc_ohmic_losses now and any others that get added later? Or treat each separately?

It looks like pvwatts_losses still creates a self.losses attribute.

Thanks for catching that. pvwatts_losses and no_extra_losses should assign values to ModelChainResults, and ModelChain._deprecated_attrs should include losses. We can address that in another issue/pr.

Do we want to think about coordinating the storage of those losses with other losses like dc_ohmic_losses now and any others that get added later? Or treat each separately?

Good question. I'd lean toward treating them separately for now unless we have a good idea of how the losses models will evolve in the future.

Sounds good. I will work on adding dc_ohmic_losses to ModelChainResults in the next commit.

I'd lean toward treating them separately for now

I agree. If losses attributes pile up in ModelChainResults we can consider creating a Losses dataclass or other mechanism to group losses together.

docs/sphinx/source/api.rst

pvlib/pvsystem.py

wholmgren

almost there...

pvlib/modelchain.py

wholmgren · 2021-03-08T20:39:31Z

pvlib/modelchain.py

@@ -1068,6 +1076,51 @@ def faiman_temp(self):
    def fuentes_temp(self):
        return self._set_celltemp(self.system.fuentes_celltemp)

+    @property
+    def dc_ohmic_model(self):
+        return self._dc_ohmic_model


any idea why codecov says this line isn't tested? It looks like it should be.

That was confusing for me as well. And now it is telling me that blank lines 1082 and 1095 are not covered, and that 1090 is not covered which it should be as there is a test specifically for dc_ohmic_model='no_loss'. I'd welcome an assist from someone more familiar with the tests on what might be leading to the failed coverage if anyone has an idea.

I ran a coverage report locally and cannot reproduce the codecov results. Adding exceptions to these lines causes errors, as you would expect.

wholmgren · 2021-03-16T17:27:09Z

pvlib/pvsystem.py

@@ -1149,6 +1166,9 @@ class Array:
        Valid strings are 'open_rack', 'close_mount', and 'insulated_back'.
        Used to identify a parameter set for the SAPM cell temperature model.

+    array_losses_parameters: None, dict or Series, default None.


sorry, should have caught this earlier.... it seems repetitive to call this parameter array_losses_parameters since it's already on an Array. I suppose there is some risk of user confusion if this and the PVSystem losses_parameters are called the same thing, but it doesn't hurt to have extra keys in these dictionaries. It's unlikely that would change in the future.

Maybe we should live with the current API on master in an alpha release or two and then maybe take up renaming in a follow up issue. I could see something like #1176 providing more clarity on this.

Thoughts @ncroft-b4 @cwhanse ?

I'm OK with either array_losses_parameters or Array.losses_parameters (and PVSystem.losses_parameters) in this PR.

I think as loss models are added to pvlib and we work those models into ModelChain, we'll end up refactoring the parameter containers.

Yeah I think my concern was making it distinguishable from the PVSystem parameter. Also, to keep compatibility with initializing a PVSystem directly, there needs to be a difference between the losses_parameters that would be passed into the Array and the ones that would stay with the PVSystem.

That makes me realize that I added array_losses_parameters to the list of keys that get passed to Array from PVSystem if an Array is not specified, but I did not add array_losses_parameters explicitly to the documentation for PVSystem or to the init. That makes sense to be because array_losses_parameters is only an Array parameter, but it does make it different from the other Array parameters that are still part of PVSystem by legacy. I'm good with it as it is but if you want to keep it uniform with the legacy parameters we could change it.

wholmgren · 2021-03-16T18:49:57Z

Ok, let's move on for now. Thanks @ncroft-b4 for the contribution and your patience as we worked through it!

wholmgren · 2021-03-16T18:51:43Z

and after I scrolled to the top to go to my notifications I realized that we don't have a whats new entry. Sorry. I can add that in another PR.

adriesse · 2021-03-17T18:58:51Z

I have just quickly scrolled through this and wanted to mention something that may be relevant to someone sometime: PVsyst models DC ohmic losses by increasing the series resistance in the single-diode model parameters. This seems like a good approach to me, and I suppose one can still do that independently of this new code.

ncroft-b4 added 3 commits January 25, 2021 13:10

Support functions for ohmic losses

5c79590

PVSystem dc ohmic losses and tests

b6fa6f6

ModelChain dc ohimc losses as tests

8b6769c

ncroft-b4 mentioned this pull request Feb 15, 2021

Change setter for losses_model to allow it to set multiple loss funct… #1084

Closed

8 tasks

ncroft-b4 and others added 2 commits February 15, 2021 10:46

Merge branch 'master' into redevelopment

18228de

updates to api.rst for dc_ohmic_losses

ab5ecff

cwhanse reviewed Feb 23, 2021

View reviewed changes

kandersolar reviewed Feb 23, 2021

View reviewed changes

pvlib/pvsystem.py Outdated Show resolved Hide resolved

pvlib/pvsystem.py Outdated Show resolved Hide resolved

pvlib/pvsystem.py Outdated Show resolved Hide resolved

pvlib/pvsystem.py Outdated Show resolved Hide resolved

Docstring formatting updates

f63a24b

ncroft-b4 commented Feb 24, 2021

View reviewed changes

ncroft-b4 added 5 commits February 24, 2021 11:42

make dc_ohms_from_percent work with other dc models

0db5afe

format fixes

a8a911e

support for multiple arrays

3c7b85d

added tests for new module parameters

8749450

modelchain tests for multiple array support

4500bc7

wholmgren reviewed Mar 3, 2021

View reviewed changes

ncroft-b4 added 2 commits March 5, 2021 12:15

fix api, document render, and use explict variables

547f6c1

resolve conflict and small format fix

68857ab

wholmgren mentioned this pull request Mar 5, 2021

ModelChain.pvwatts_losses creates self.losses instead of ModelChainResult.losses #1184

Closed

ncroft-b4 added 2 commits March 5, 2021 14:14

dc_ohmic_losses as results attribute

26be1fe

fix tests and None model

f96ac03

wholmgren reviewed Mar 8, 2021

View reviewed changes

wholmgren added this to the 0.9.0 milestone Mar 10, 2021

wholmgren added the enhancement label Mar 10, 2021

ncroft-b4 added 2 commits March 15, 2021 14:34

remove none as model option

ba92da6

remove none from tests

19728cd

wholmgren added the SPI DOE SETO Solar Performance Insight project label Mar 16, 2021

wholmgren approved these changes Mar 16, 2021

View reviewed changes

cwhanse approved these changes Mar 16, 2021

View reviewed changes

wholmgren merged commit 2a1ed55 into pvlib:master Mar 16, 2021

lboeman mentioned this pull request Mar 16, 2021

Add support for DC Ohmic losses SolarPerformanceInsight/solarperformanceinsight#179

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DC Ohmic Losses for pvsystem and modelchain #1168

DC Ohmic Losses for pvsystem and modelchain #1168

ncroft-b4 commented Feb 15, 2021 •

edited by wholmgren

Loading

cwhanse Feb 23, 2021

ncroft-b4 Feb 24, 2021

cwhanse Feb 24, 2021

ncroft-b4 Feb 24, 2021

kandersolar left a comment

ncroft-b4 Feb 24, 2021

cwhanse Feb 24, 2021

ncroft-b4 Feb 24, 2021

wholmgren left a comment

wholmgren Mar 3, 2021

ncroft-b4 Mar 5, 2021

wholmgren Mar 5, 2021

ncroft-b4 Mar 5, 2021

cwhanse Mar 5, 2021

wholmgren left a comment

wholmgren Mar 8, 2021

ncroft-b4 Mar 15, 2021

wholmgren Mar 16, 2021

wholmgren Mar 16, 2021

cwhanse Mar 16, 2021

ncroft-b4 Mar 16, 2021

wholmgren commented Mar 16, 2021

wholmgren commented Mar 16, 2021

adriesse commented Mar 17, 2021

DC Ohmic Losses for pvsystem and modelchain #1168

DC Ohmic Losses for pvsystem and modelchain #1168

Conversation

ncroft-b4 commented Feb 15, 2021 • edited by wholmgren Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kandersolar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wholmgren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wholmgren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wholmgren commented Mar 16, 2021

wholmgren commented Mar 16, 2021

adriesse commented Mar 17, 2021

ncroft-b4 commented Feb 15, 2021 •

edited by wholmgren

Loading