Create inverter.py #886

cwhanse · 2020-02-11T22:11:15Z

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.

Creates inverter.py

pvsystem.snlinverter to inverter.sandia
pvsystem.adrinverter to inverter.adr
pvsystem.pvwatts_ac to inverter.pvwatts

adriesse · 2020-02-13T21:31:37Z

I fully support the new module, but I wonder about the naming. Perhaps 'efficiency' or 'power' should be part of it, since that's what the current functions model? I can imagine inverter-related functions that model other aspects.

…nverter

pvlib/tests/test_pvsystem.py

…nverter

cwhanse · 2020-06-10T22:53:53Z

Ready for review

docs/sphinx/source/whatsnew/v0.8.0.rst

pvlib/inverter.py

pvlib/pvsystem.py

kandersolar · 2020-06-12T01:09:07Z

pvlib/tests/test_modelchain.py

@@ -447,22 +447,29 @@ def acdc(mc):
    mc.ac = mc.dc


+# TODO in v0.9: remove 'snlinverter', 'adrinverter', 'pvwatts'


Same here about 'pvwatts' being removed

I've edited the TODO comment to drop the 'snlinverter' and 'adrinverter' values for ModelChain.ac_model.

There's a loose thread outside of this PR, in that PVSystem.snlinverter, PVSystem.adrinverter and PVSystem.pvwatts_ac methods aren't renamed. I couldn't come up with a naming scheme for the PVSystem methods that I liked, when the methods wrap functions in a different module. The method name needs to package both the method purpose (inverter model) and the model (snl, adr, pvwatts).

cwhanse · 2020-06-15T19:34:19Z

Test failures are unrelated (NREL server not responding), otherwise I think I've addressed the review comments (thanks @kanderso-nrel )

cwhanse · 2020-06-25T19:23:28Z

@kanderso-nrel can you see if the NSRDB PSM3 data service is offline? Looks that way to me.

Test are passing except get_psm3_key

kandersolar · 2020-06-25T19:44:36Z

I'm getting tests passing except for get_psm3_tmy too. On first glance, it seems like a few of the data columns have changed for that TMY dataset, possibly the date labeling too. Not sure what to make of that -- I had the impression that tmy-2017 was static and should never change. I'll open an issue on the NREL github to see if this was an unintentional change.

kandersolar

A few more comments. I wish github would allow comments on code that wasn't changed in the PR, but absent that here is a list:

lines 2444, 2446 in pvsystem.py still reference pvsystem.pvwatts_ac.
git grep -n snlinverter shows a handful of docs files still referencing pvsystem.snlinverter. I checked the other two and didn't see any offenders.

docs/sphinx/source/introtutorial.rst:99:        ac = pvlib.pvsystem.snlinverter(dc['v_mp'], dc['p_mp'], inverter)
docs/sphinx/source/introtutorial.rst:245:        ac = localized_system.snlinverter(dc['v_mp'], dc['p_mp'])
docs/tutorials/forecast.ipynb:8162:       "  ac_model: snlinverter\n",
docs/tutorials/forecast_to_power.ipynb:879:    "p_ac = pvsystem.snlinverter(sapm_out.v_mp, sapm_out.p_mp, sapm_inverter)\n",
docs/tutorials/pvsystem.ipynb:576:    "### snlinverter"
docs/tutorials/pvsystem.ipynb:1359:    "pacs = pvsystem.snlinverter(vdcs, pdcs, inverters['ABB__MICRO_0_25_I_OUTD_US_208_208V__CEC_2014_'])\n",
docs/tutorials/tmy_to_power.ipynb:1187:    "p_acs['sapm'] = pvlib.pvsystem.snlinverter(sapm_out.v_mp, sapm_out.p_mp, sapm_inverter)\n",
docs/tutorials/tmy_to_power.ipynb:1188:    "p_acs['sd'] = pvlib.pvsystem.snlinverter(single_diode_out.v_mp, single_diode_out.p_mp, sapm_inverter)\n",

pvlib/modelchain.py

docs/sphinx/source/api.rst

pvlib/inverter.py

kandersolar · 2020-06-26T03:02:09Z

pvlib/inverter.py

+    Pnt       AC power consumed by inverter at night (night tare) to
+              maintain circuitry required to sense PV array voltage. [W]
+
+    =======   ============================================================


Sphinx still doesn't like this. I'm not if all of these steps are necessary, but I got it working locally for me by:

Adding spacing between terms and definitions so that there is no overlap between columns (the ADRCoefficients term sticks out pretty far)

Extending the left ==== strings so that they completely cover the column width

Removing the empty line between Pnt and the trailing line.

Here's the text that works for me (with some text reflows to stay under 80 chars):

=============== ======================================================== Column Description =============== ======================================================== Pnom Nominal DC power, typically the DC power needed to produce maximum AC power output. [W] Vnom Nominal DC input voltage. Typically the level at which the highest efficiency is achieved. [V] Vmax Maximum DC input voltage. [V] Vmin Minimum DC input voltage. [V] Vdcmax . [V] MPPTHi Maximum DC voltage for MPPT range. [V] MPPTLow Minimum DC voltage for MPPT range. [V] Pacmax Maximum AC output power, used to clip the output power if needed. [W] ADRCoefficients A list of 9 coefficients that capture the influence of input voltage and power on inverter losses, and thereby efficiency. Corresponds to terms from [1]_ (in order): :math:`b_{0,0}, b_{1,0}, b_{2,0}, b_{0,1}, b_{1,1}, b_{2,1}, b_{0,2}, b_{1,2}, b_{1,2}`. See [1]_ for the use of each coefficient and the associated unit. Pnt AC power consumed by inverter at night (night tare) to maintain circuitry required to sense PV array voltage. [W] =============== ========================================================

kandersolar · 2020-06-26T03:02:52Z

pvlib/inverter.py

+    Pnt       AC power consumed by inverter at night (night tare) to
+              maintain circuitry required to sense PV array voltage. [W]
+
+    =======   ============================================================


Two more notes: Vdcmax is still undefined and b_{1,2} is repeated in ADRCoefficients, presumably should be b_{2,2}?

docs/sphinx/source/whatsnew/v0.8.0.rst

cwhanse · 2020-06-26T16:06:23Z

git grep -n snlinverter shows a handful of docs files still referencing pvsystem.snlinverter. I checked the other two and didn't see any offenders.

Many thanks for checking this. I've made these corrections where the function is referenced; other instances are references to PVSystem and ModelChain methods that aren't being renamed in this PR.

kandersolar

Agreed that most of the output of git grep -n snlinverter is from ModelChain or the PVSystem method, but I think this subset is still the pvsystem function:

docs/tutorials/forecast_to_power.ipynb:879:    "p_ac = pvsystem.snlinverter(sapm_out.v_mp, sapm_out.p_mp, sapm_inverter)\n",
docs/tutorials/pvsystem.ipynb:576:    "### snlinverter"
docs/tutorials/pvsystem.ipynb:1359:    "pacs = pvsystem.snlinverter(vdcs, pdcs, inverters['ABB__MICRO_0_25_I_OUTD_US_208_208V__CEC_2014_'])\n",

Otherwise LGTM!

pvlib/inverter.py

cwhanse · 2020-06-26T16:48:21Z

docs/tutorials/pvsystem.ipynb:576: "### snlinverter"

What does the ### formatting do? I assumed this was just an internal label or link.

kandersolar · 2020-06-26T16:52:30Z

docs/tutorials/pvsystem.ipynb:576: "### snlinverter"

What does the ### formatting do? I assumed this was just an internal label or link.

That's inside a markdown cell, just making a small header:

It's not code so nbd to leave as-is.

…nverter

cwhanse · 2020-07-01T20:43:20Z

@wholmgren ok to merge?

wholmgren

Sorry for the delay @cwhanse. A handful of minor things but looks very close.

Thanks @kanderso-nrel for the detailed review.

docs/sphinx/source/api.rst

wholmgren · 2020-07-04T17:53:12Z

docs/sphinx/source/whatsnew/v0.8.0.rst

+   - :py:func:`pvlib.pvsystem.snlinverter` is now :py:func:`pvlib.inverter.sandia`
+   - :py:func:`pvlib.pvsystem.pvwatts_ac` is now :py:func:`pvlib.inverter.pvwatts`
+   - :py:func:`pvlib.pvsystem.adrinverter` is now :py:func:`pvlib.inverter.adr`
+* ModelChain.ac_model now accepts `'sandia'`, `'pvwatts'` and `'adr'` for the


does this line render as you intend it to?

'sandia' is intended to display as a string value. Should it be marked differently?

The current rendering is fine with me (shown below), but the meaning of single backticks is context dependent in rst. If you want to keep these in italics, I prefer *'sandia'*. Also fine to format as code (e.g. double backtick 'sandia' double backtick)

pvlib/inverter.py

wholmgren · 2020-07-04T17:56:01Z

pvlib/modelchain.py

@@ -56,7 +56,7 @@ def basic_chain(times, latitude, longitude,
        See temperature.sapm_cell for details.

    inverter_parameters : None, dict or Series
-        Inverter parameters as defined by the CEC. See pvsystem.snlinverter for
+        Inverter parameters as defined by the CEC. See inverter.sandia for


noting the open comment that may have been buried

pvlib/modelchain.py

pvlib/pvsystem.py

wholmgren · 2020-07-04T18:07:52Z

pvlib/tests/test_modelchain.py

+                      'sandia': 'snlinverter',
+                      'adrinverter': 'adrinverter',
+                      'adr': 'adrinverter',
+                      'pvwatts': 'pvwatts_ac'}
    system = ac_systems[ac_model]



Ideally we would test that the deprecation warning is raised if the ac method name is snlinverter or adrinverter. Ok with me to skip it though since the code looks correct and is not too complicated.

I'll add a test for the deprecation warning if ModelChain.ac_model is assigned 'snlinverter' or 'adrinverter'

The ModelChain and PVSystem methods are still snlinverter and adrinverter. Do we want to rename those as well? I favor doing that, but in a separate PR.

I agree we should rename the methods in a separate PR.

pvlib/tests/test_modelchain.py

wholmgren

minor doc comments then ready to merge

wholmgren · 2020-07-07T19:26:51Z

docs/sphinx/source/whatsnew/v0.8.0.rst

@@ -3,15 +3,25 @@
 v0.8.0 (Month day, year)
 -------------------------

+API Changes with Deprecations
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+* Moved functions related to inverters from `pvsystem.py` to `inverter.py`.


Suggested change

* Moved functions related to inverters from `pvsystem.py` to `inverter.py`.

* Moved functions related to inverters from ``pvsystem.py`` to ``inverter.py``.

wholmgren · 2020-07-07T19:32:12Z

docs/sphinx/source/whatsnew/v0.8.0.rst

+* Attribute :py:attribute:`modelchain.ModelChain.ac_model` now accepts ``sandia``,
+  ``pvwatts`` and ``adr`` for the inverter models.  (:pull:`886`)


I'm guessing the vast majority of people set this through the ac_model kwarg rather than the attribute. So I would link to :py:class:`pvlib.modelchain.ModelChain` instead. Also suggest adding ' within the code blocks.

wholmgren · 2020-07-07T19:35:16Z

docs/sphinx/source/whatsnew/v0.8.0.rst

@@ -23,6 +33,8 @@ Testing
 ~~~~~~~
 * Decorator :py:func:`pvlib.conftest.fail_on_pvlib_version` can now be
  applied to functions that require args or kwargs. (:pull:`973`)
+* Test added for :py:meth:`ModelChain` to confirm ValueError when


use same link code as commented above

wholmgren · 2020-07-07T19:35:48Z

pvlib/inverter.py

+def adr(v_dc, p_dc, inverter, vtol=0.10):
+    r'''
+    Converts DC power and voltage to AC power using Anton Driesse's
+    Grid-Connected PV Inverter efficiency model


Suggested change

Grid-Connected PV Inverter efficiency model

Grid-Connected PV Inverter efficiency model.

grid-connected inverter?

create inverter.py, move code, add adrinverter fixture

e9fe39c

cwhanse added the api label Feb 11, 2020

cwhanse added this to the 0.8.0 milestone Feb 11, 2020

cwhanse added 3 commits February 11, 2020 15:24

stickler, adjust PVSystem methods

4be25ba

update modelchain, test fixes

904595b

sticker

1769295

cwhanse mentioned this pull request Mar 9, 2020

Rethinking pvlib.ivtools #931

Closed

cwhanse mentioned this pull request Apr 23, 2020

v0.8.0 release planning #955

Closed

cwhanse added 5 commits May 21, 2020 10:18

Merge branch 'master' of https://github.com/pvlib/pvlib-python into i…

1dd3e7b

…nverter

docstring improvements

06cb965

more docstring work

d13d162

lint, kludge inverter.test_deprecated_09

592d33b

lint, kludge test_deprecated_09 x2

1bb1567

cwhanse commented May 21, 2020

View reviewed changes

pvlib/tests/test_pvsystem.py Outdated Show resolved Hide resolved

cwhanse added 9 commits June 10, 2020 12:38

Merge branch 'master' of https://github.com/pvlib/pvlib-python into i…

a27bbaf

…nverter

use new conftest capability

d371713

correct use of fixture

f9f8ae7

add test for invalid ac model string

f6d6f77

update whatsnew

d0e811f

update api.rst

f630f05

extend test_ac_models to new key values

78a7611

more for whatsnew

4a19d67

correct modelchain.ac_model test

ea90bec

cwhanse changed the title ~~WIP: create inverter.py~~ Create inverter.py Jun 10, 2020

test correction

1eb75cd

kandersolar reviewed Jun 12, 2020

View reviewed changes

edits from review

e471ec2

cwhanse mentioned this pull request Jun 25, 2020

Implement a function to calculate inverter model parameters #975

Closed

kandersolar mentioned this pull request Jun 25, 2020

NSRDB API returning bad data for tmy-2017 NREL/developer.nrel.gov#159

Closed

kandersolar reviewed Jun 26, 2020

View reviewed changes

improvements from review

c85ab82

kandersolar approved these changes Jun 26, 2020

View reviewed changes

pvlib/inverter.py Outdated Show resolved Hide resolved

pvlib/inverter.py Outdated Show resolved Hide resolved

more improvements from review

dd40006

kandersolar mentioned this pull request Jun 30, 2020

Update tutorials for pvlib >=0.7 #995

Closed

Merge branch 'master' of https://github.com/pvlib/pvlib-python into i…

89c3460

…nverter

wholmgren reviewed Jul 4, 2020

View reviewed changes

improvements from review

7663321

wholmgren reviewed Jul 6, 2020

View reviewed changes

pvlib/tests/test_modelchain.py Outdated Show resolved Hide resolved

wholmgren reviewed Jul 6, 2020

View reviewed changes

pvlib/tests/test_modelchain.py Show resolved Hide resolved

cwhanse added 6 commits July 6, 2020 12:08

improvements from review

ff13e7e

Merge branch 'master' into inverter

da5dc15

last cleanup

75d818b

last cleanup part 2

9a9f105

last cleanup part 3

9be21e4

py:attribute in whatsnew

33cddd2

wholmgren reviewed Jul 7, 2020

View reviewed changes

cwhanse added 2 commits July 7, 2020 13:50

docstring improvements

1e07351

one more

6ba55aa

cwhanse merged commit 97c6698 into pvlib:master Jul 7, 2020

cwhanse mentioned this pull request Jul 7, 2020

Consider replacing inverter methods in PVSystem with a single method with a kwarg #998

Closed

cwhanse deleted the inverter branch July 9, 2020 18:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create inverter.py #886

Create inverter.py #886

cwhanse commented Feb 11, 2020 •

edited

Loading

adriesse commented Feb 13, 2020

cwhanse commented Jun 10, 2020

kandersolar Jun 12, 2020

cwhanse Jun 12, 2020

cwhanse commented Jun 15, 2020

cwhanse commented Jun 25, 2020 •

edited

Loading

kandersolar commented Jun 25, 2020

kandersolar left a comment

kandersolar Jun 26, 2020

kandersolar Jun 26, 2020

cwhanse commented Jun 26, 2020

kandersolar left a comment

cwhanse commented Jun 26, 2020

kandersolar commented Jun 26, 2020

cwhanse commented Jul 1, 2020

wholmgren left a comment

wholmgren Jul 4, 2020

cwhanse Jul 6, 2020

wholmgren Jul 6, 2020

wholmgren Jul 4, 2020

wholmgren Jul 4, 2020

cwhanse Jul 6, 2020

wholmgren Jul 6, 2020

wholmgren left a comment

wholmgren Jul 7, 2020

wholmgren Jul 7, 2020

wholmgren Jul 7, 2020

wholmgren Jul 7, 2020

wholmgren Jul 7, 2020

		@@ -447,22 +447,29 @@ def acdc(mc):
		mc.ac = mc.dc


		# TODO in v0.9: remove 'snlinverter', 'adrinverter', 'pvwatts'

	* Moved functions related to inverters from `pvsystem.py` to `inverter.py`.
	* Moved functions related to inverters from ``pvsystem.py`` to ``inverter.py``.

		* Attribute :py:attribute:`modelchain.ModelChain.ac_model` now accepts ``sandia``,
		``pvwatts`` and ``adr`` for the inverter models. (:pull:`886`)

	Grid-Connected PV Inverter efficiency model
	Grid-Connected PV Inverter efficiency model.

Create inverter.py #886

Create inverter.py #886

Conversation

cwhanse commented Feb 11, 2020 • edited Loading

adriesse commented Feb 13, 2020

cwhanse commented Jun 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwhanse commented Jun 15, 2020

cwhanse commented Jun 25, 2020 • edited Loading

kandersolar commented Jun 25, 2020

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

cwhanse commented Jun 26, 2020

kandersolar left a comment

Choose a reason for hiding this comment

cwhanse commented Jun 26, 2020

kandersolar commented Jun 26, 2020

cwhanse commented Jul 1, 2020

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

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

cwhanse commented Feb 11, 2020 •

edited

Loading

cwhanse commented Jun 25, 2020 •

edited

Loading