add Mount classes #1176

kandersolar · 2021-02-23T18:12:23Z

Closes make Array play nicely with fixed tilt systems and trackers #1109
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.
Deprecate pvsystem.SingleAxisTracker?

A draft implementation of the Mount classes proposed in #1146 (comment). I updated the pvsystem tests, but not anywhere else yet. Note: I made a new module (pvlib.mounts) as a quick fix for a circular import issue between pvlib.pvsystem and pvlib.tracking, but I'm not sure that's the right fix in the long run.

Sorry if this has already been discussed elsewhere, but: I suspect a consequence of moving tracking calculations down to Array, or more specifically Array.mount, is that SingleAxisTracker (the subclass of PVSystem) becomes unnecessary.

Example usage:

Click to expand!

import pandas as pd
import matplotlib.pyplot as plt

from pvlib.location import Location
from pvlib.pvsystem import Array, PVSystem
from pvlib.mounts import FixedMount, SingleAxisTrackerMount
from pvlib.modelchain import ModelChain

times = pd.date_range('2019-06-01', '2019-06-02', freq='15min', tz='Etc/GMT+5')
location = Location(40, -80)
weather = location.get_clearsky(times)
weather['temp_air'] = 1
weather['wind_speed'] = 0

kwargs = dict(
    module_parameters={'pdc0': 1000, 'gamma_pdc': -0.004, 'b': 0.05},
    temperature_model_parameters={'a': -3.56, 'b': -0.075, 'deltaT': 3}
)
arrays = [
    Array(FixedMount(20, 180), **kwargs),
    Array(SingleAxisTrackerMount(0, 180, 60, True, 0.5, 0), **kwargs)
]
system = PVSystem(arrays, inverter_parameters={'pdc0': 2000})
mc = ModelChain(system, location, spectral_model='no_loss')
mc.run_model(weather)
for array, dc in zip(arrays, mc.results.dc):
    dc.plot(label='dc: ' + array.mount.__class__.__name__)

mc.results.ac.plot(label='ac')
plt.legend()
plt.show()

wholmgren · 2021-02-23T18:37:26Z

Thanks! This looks like a cleaner solution to me and I suspect it will be easier to maintain.

I made a new module (pvlib.mounts) as a quick fix for a circular import issue between pvlib.pvsystem and pvlib.tracking, but I'm not sure that's the right fix in the long run.

I ran into this too and put the import in a method: https://github.com/pvlib/pvlib-python/pull/1146/files#diff-49dca2a56e76272f399adc5e162e9e026ddb8b39ab4136d1d106e188691c7be7R1616

I suspect a consequence of moving tracking calculations down to Array, or more specifically Array.mount, is that SingleAxisTracker (the subclass of PVSystem) becomes unnecessary.

Yes, that is part of my goal #1109 (comment). Another win for composition over inheritance.

kandersolar · 2021-02-24T00:57:54Z

I'm not sure what the best move is here for SingleAxisTracker. If we're planning to deprecate it, it seems reasonable to me to not put effort into making it compatible with new features like passing it an Array instead of the usual parameter list, so in an effort to keep things simple I've reverted its interface back to the pre-Array version while using the new stuff under the hood. Open to suggestions.

wholmgren

I agree that we only need SingleAxisTracker to work as it did in v0.8 through a v0.9 deprecation period - it doesn't need any new functionality.

@wfvining it would be great to get your feedback on this draft PR.

pvlib/pvsystem.py

wholmgren · 2021-02-24T16:36:25Z

pvlib/pvsystem.py

+            self.max_angle, self.backtrack,
+            self.gcr, self.cross_axis_tilt
+        )
+        return tracking_data


still thinking about interface promises... get_orientation promises to return something dict-like that includes keys surface_tilt and surface_azimuth.

pvlib/pvsystem.py

pvlib/modelchain.py

pvlib/pvsystem.py

kandersolar · 2021-03-07T19:47:07Z

I don't think it's possible (or particularly useful) to test the pass body of AbstractMount, so coverage here isn't going to be exactly 100%.

@cwhanse you brought up racking_model for the SAPM thermal model. I think there's a similar situation with fuentes and module_height. Should racking_model and module_height be moved down to the mount level as optional parameters?

wholmgren

I think the only big thing we're missing on this PR is @cwhanse's opinion of the approach. Of course other opinions are welcome too.

pvlib/pvsystem.py

cwhanse

I think move both racking_model and module_height to Mount and see how this progresses.

pvlib/pvsystem.py

cwhanse · 2021-03-11T02:51:35Z

pvlib/pvsystem.py

+class AbstractMount(ABC):
+
+    @abstractmethod
+    def calculate_orientation(self, solar_zenith, solar_azimuth):


maybe get_module_orientation? Or get_module_position? get_orientation?

pvlib methods prefixed with 'get' typically have 'how' or 'method' kwargs. Python discourages the prefix in most cases.

~~What is this method intended to return? tilt and azimuth I assume?~~ [nvm I read the code again, short memory I guess]. calculate seems like a long way to say get to me.

After thinking a little more about this... I had an unfair allergic reaction to get in this case. It's more correct to say that getter methods are not pythonic. This isn't a getter method, so we're ok. We use get for a lot of things that do calculations, so there's still consistency. I'm now neutral on the prefix.

kandersolar · 2021-06-17T15:22:09Z

pvlib/pvsystem.py

+                        "specify it as a key in temperature_model_parameters "
+                        "or as an attribute of the Array's Mount."
+                    )
+                    raise ValueError(msg)


I'm not sure what to do about the various places to look for surface_tilt now that the PVSystem and its Mount aren't guaranteed to have a surface_tilt attribute. Is it reasonable to remove this complexity and only look in temperature_model_parameters?

Is it reasonable to remove this complexity and only look in temperature_model_parameters?

Sounds good to me.

Probably a bad idea: An alternative would be calling get_orientation with something like 0, 0 - doesn't matter for fixed tilt systems and it would make most single axis trackers return 0.

Definitely not interested in adding solar position kwargs to this method.

kandersolar · 2021-06-23T23:39:49Z

Finally marking this ready for review. Figured I'd wait to do the whatsnew until folks have a chance to take a look and make sure no other big stuff should go into this PR.

wholmgren

This is great!

Need to update introtutorial.rst Object Oriented text and code. Can be a follow up PR if you make a new issue for it.

pvsystem.rst would benefit from an example with SingleAxisTrackerMount. Follow up issue/PR ok.

+1 on deprecating SingleAxisTracker. Follow up issue/PR ok. We shouldn't forget to address pvsystem.rst SingleAxisTracker section in that process.

There are a few tests in test_modelchain.rst that use SingleAxisTracker. Most if not all of that should be retested using the new mount. That could be an argument for doing the deprecation now. But copy/paste/modify/punt is fine too.

docs/sphinx/source/api.rst

pvlib/pvsystem.py

kandersolar · 2021-07-21T20:53:07Z

In the interest of not holding this up any longer (sorry again), I think let's push the various .rst updates to other PRs. Stickler is complaining about line length in the tests, I hope we can ignore those. The Azure failures seem like an issue on their end and so hopefully also ok to ignore.

pvlib/tests/test_modelchain.py

wholmgren · 2021-07-22T03:06:14Z

pvlib/tracking.py



+@deprecated('0.9.0', alternative='PVSystem with SingleAxisTrackingMount')


hmm seeing the difference with the class name has me wondering... SingleAxisTrackingMount or SingleAxisTrackerMount?

I surveyed n=2 pvlib users and both preferred SingleAxisTrackerMount. I will make the change.

Turns out it was already called SingleAxisTrackerMount and I just called it the wrong thing in the deprecation message 🤦‍♂️

At least we got some data out of it!

Co-authored-by: Will Holmgren <william.holmgren@gmail.com>

…b-python into mount_classes

wholmgren

@cwhanse ok to merge?

cwhanse · 2021-07-22T18:38:07Z

pvlib/pvsystem.py

+        Valid strings are 'open_rack', 'close_mount', and 'insulated_back'.
+        Used to identify a parameter set for the SAPM cell temperature model.
+
+    module_height : float, optional


Here, I don't understand module_height; does it not change with tracker rotation? Maybe axis_height and an internal calculation would produce module_height as an attribute, not an input parameter. In FixedMount, module_height is merely awkward, as the measurement that seems more likely to be at hand is the height of the bottom of the module above ground.

I'm OK merging this as is and improving in later PRs.

Good points. +1 to improving in later PRs.

wholmgren · 2021-07-27T03:06:03Z

thanks for the huge refactor @kanderso-nrel!

🥳 🥳 🥳

kandersolar added 2 commits February 23, 2021 10:20

add Mount classes, incorporate into Array and PVSystem

cb3c207

update pvsystem tests

8a6d0e6

kandersolar added 3 commits February 23, 2021 15:46

delete mounts module

6d4240e

fix modelchain tests

fc4064b

some modifications to SingleAxisTracker

e8f1404

wholmgren reviewed Feb 24, 2021

View reviewed changes

wfvining reviewed Feb 24, 2021

View reviewed changes

pvlib/pvsystem.py Outdated Show resolved Hide resolved

wholmgren mentioned this pull request Feb 26, 2021

Confusing Note about Single Axis SolarPerformanceInsight/solarperformanceinsight#155

Closed

kandersolar added 2 commits February 27, 2021 15:11

changes from review

02a2b27

stickler

0be45ae

kandersolar commented Feb 27, 2021

View reviewed changes

pvlib/modelchain.py Outdated Show resolved Hide resolved

kandersolar mentioned this pull request Feb 28, 2021

Remove orientation_strategy from Modelchain #1181

Merged

8 tasks

wfvining reviewed Mar 1, 2021

View reviewed changes

pvlib/pvsystem.py Show resolved Hide resolved

kandersolar added 3 commits March 1, 2021 21:36

Merge branch 'master' into mount_classes

9406ccd

use dataclasses for mounts

e1bdc67

update tests

61650e9

kandersolar mentioned this pull request Mar 7, 2021

irradiance.aoi can return NaN when module orientation is perfectly aligned with solar position #1185

Closed

kandersolar added 7 commits March 7, 2021 10:30

update docs

fc47003

whatsnew

887fd3a

test mount classes

ecc4737

stickler

cbb41e1

more tests

9e663d2

another test

105edb7

fix typo

6d32c41

wholmgren reviewed Mar 7, 2021

View reviewed changes

pvlib/pvsystem.py Outdated Show resolved Hide resolved

pvlib/pvsystem.py Outdated Show resolved Hide resolved

pvlib/pvsystem.py Show resolved Hide resolved

cwhanse reviewed Mar 11, 2021

View reviewed changes

clean up AbstractMount

4a6347e

kandersolar commented Jun 17, 2021

View reviewed changes

kandersolar added 5 commits June 23, 2021 16:51

remove fuentes override complexity

59547f5

stickler

bed27c9

update RST pages

1103f99

Merge remote-tracking branch 'upstream/master' into mount_classes

e6504cd

revert unnecessary docs change

73aba9a

kandersolar marked this pull request as ready for review June 23, 2021 23:35

wholmgren added this to the 0.9.0 milestone Jun 25, 2021

wholmgren added api enhancement labels Jun 25, 2021

wholmgren reviewed Jun 25, 2021

View reviewed changes

docs/sphinx/source/api.rst Show resolved Hide resolved

pvlib/pvsystem.py Show resolved Hide resolved

pvlib/pvsystem.py Outdated Show resolved Hide resolved

pvlib/pvsystem.py Show resolved Hide resolved

wholmgren mentioned this pull request Jun 25, 2021

add SingleAxisArray. Array to BaseArray, FixedTiltArray #1146

Closed

8 tasks

kandersolar added 6 commits June 28, 2021 20:54

add link to pvsystem and modelchain pages in api listing

f3ff722

other changes from review

9c0c3ff

get module_height from mount instead of temperature_model_parameters

35da280

coverage for fuentes module_height parameter

1a43ea8

deprecate SingleAxisTracker

c1738e9

suppress SAT deprecation warnings in tests

781b133

wholmgren reviewed Jul 22, 2021

View reviewed changes

kandersolar and others added 3 commits July 22, 2021 09:43

Apply suggestions from code review

22341ec

Co-authored-by: Will Holmgren <william.holmgren@gmail.com>

Tracking -> Tracker

6df8408

Merge branch 'mount_classes' of https://github.com/kanderso-nrel/pvli…

506edbc

…b-python into mount_classes

wholmgren approved these changes Jul 22, 2021

View reviewed changes

cwhanse approved these changes Jul 22, 2021

View reviewed changes

wholmgren merged commit f13d1b1 into pvlib:master Jul 27, 2021

wholmgren mentioned this pull request Jul 27, 2021

module_height documentation #1263

Closed

kandersolar deleted the mount_classes branch July 27, 2021 14:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add Mount classes #1176

add Mount classes #1176

kandersolar commented Feb 23, 2021 •

edited

Loading

wholmgren commented Feb 23, 2021

kandersolar commented Feb 24, 2021

wholmgren left a comment

wholmgren Feb 24, 2021

kandersolar commented Mar 7, 2021

wholmgren left a comment

cwhanse left a comment

cwhanse Mar 11, 2021 •

edited

Loading

wholmgren Mar 11, 2021

cwhanse Mar 11, 2021 •

edited

Loading

wholmgren Mar 11, 2021

kandersolar Jun 17, 2021

wholmgren Jun 21, 2021

kandersolar commented Jun 23, 2021

wholmgren left a comment

kandersolar commented Jul 21, 2021

wholmgren Jul 22, 2021

kandersolar Jul 22, 2021

kandersolar Jul 22, 2021

wholmgren Jul 22, 2021

wholmgren left a comment

cwhanse Jul 22, 2021

kandersolar Jul 22, 2021

wholmgren commented Jul 27, 2021



		@deprecated('0.9.0', alternative='PVSystem with SingleAxisTrackingMount')

add Mount classes #1176

add Mount classes #1176

Conversation

kandersolar commented Feb 23, 2021 • edited Loading

wholmgren commented Feb 23, 2021

kandersolar commented Feb 24, 2021

wholmgren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kandersolar commented Mar 7, 2021

wholmgren left a comment

Choose a reason for hiding this comment

cwhanse left a comment

Choose a reason for hiding this comment

cwhanse Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwhanse Mar 11, 2021 • edited 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 commented Jun 23, 2021

wholmgren left a comment

Choose a reason for hiding this comment

kandersolar commented Jul 21, 2021

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

wholmgren commented Jul 27, 2021

kandersolar commented Feb 23, 2021 •

edited

Loading

cwhanse Mar 11, 2021 •

edited

Loading

cwhanse Mar 11, 2021 •

edited

Loading