Skip to content
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

add SingleAxisArray. Array to BaseArray, FixedTiltArray #1146

Closed
wants to merge 6 commits into from

Conversation

wholmgren
Copy link
Member

  • 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.

Initial implementation of the ideas first discussed in #1076 (comment) and #1109. I'm looking for feedback on this approach before integrating it with ModelChain. Also fine to discard it if we decide it's not the right approach or the right time for it.

As first discussed in the linked issues, this pattern leads to repeated calls to tracking.singleaxis: one call to get AOI and another to get POA irradiance. Add yet another call if you want to save the actual tracking data like we do in ModelChainResults. We could get around the repeated calls by adding a tracking_data=None kwarg to the methods (if tracking_data is None: tracking_data = self.singleaxis()). That wouldn't be horrible, but it also doesn't look great to me. In principle we could add a cache to SingleAxisArray, but I'd rather leave that to other libraries to do right.

@wholmgren
Copy link
Member Author

It actually mostly works without changing anything in ModelChain - the only issue is that ModelChain.results.tracking is not defined. The example below shows mixing a fixed tilt array and a single axis tracker array. It also works with multiple single axis tracker arrays. Note this example makes use of the pvwatts power scaling recently introduced in another PR.

import pandas as pd

import matplotlib.pyplot as plt

from pvlib import temperature, pvsystem, location, modelchain

temperature_model_parameters = temperature.TEMPERATURE_MODEL_PARAMETERS['sapm']['open_rack_glass_polymer']
module_parameters = {'pdc0': 240, 'gamma_pdc': -0.003}
inverter_parameters = {'pdc0': 100000}
fixed_array = pvsystem.FixedTiltArray(
    module_parameters=module_parameters,
    temperature_model_parameters=temperature_model_parameters,
    modules_per_string=60,
    strings=4
)
tracking_array = pvsystem.SingleAxisArray(
    module_parameters=module_parameters,
    temperature_model_parameters=temperature_model_parameters,
    strings=4,
    modules_per_string=60
)
mixed_system = pvsystem.PVSystem(
    arrays=[fixed_array, tracking_array],
    inverter_parameters=inverter_parameters
)
loc = location.Location(32.2, -110.9, altitude=700)
times = pd.date_range(start='20201201', end='20201203', freq='5min', tz='America/Phoenix')
weather = loc.get_clearsky(times)
mc_kwargs = dict(
    aoi_model='physical',
    spectral_model='no_loss',
    ac_model='pvwatts_multi'
)
mc = modelchain.ModelChain(mixed_system, loc, **mc_kwargs)
mc.run_model(weather)

fig, ax = plt.subplots()
mc.results.dc[0].plot()
fig.savefig('mixed_dc_fixed.png')

fig, ax = plt.subplots()
mc.results.dc[1].plot()
fig.savefig('mixed_dc_tracker.png')

fig, ax = plt.subplots()
mc.results.ac.plot()
fig.savefig('mixed_ac.png')

DC power

mixed_dc_fixed
mixed_dc_tracker

AC power

mixed_ac

@wholmgren
Copy link
Member Author

Some scope creep as I wanted to explore adding type hints to the object layer to hopefully make the complicated code more robust during this proposed refactor. I'm also hoping it will help us as we and others develop other projects that use pvlib. I was kind of skeptical going into it but after playing with them in my IDE and mypy I think it's a win. If we do want to implement type hints, I'm open to reimplementing them in a new PR with the current object layer before continuing with this refactor.

Output of mypy pvlib is in details below (I'm not currently using numpy 1.20 which includes its own type hints and thus could expose issues in pvlib).

mypy pvlib
pvlib/spa_c_files/spa_py_example.py:1: error: Cannot find implementation or library stub for module named 'spa_py'
pvlib/ivtools/sde.py:20: error: Incompatible types in assignment (expression has type "None", variable has type "int")
pvlib/iotools/ecmwf_macc.py:11: error: Name 'netCDF4' already defined (possibly by an import)
pvlib/spa.py:991: error: Not all arguments converted during string formatting
pvlib/_version.py:54: error: Need type annotation for 'LONG_VERSION_PY' (hint: "LONG_VERSION_PY: Dict[<type>, <type>] = ...")
pvlib/spa_c_files/setup.py:6: error: Skipping analyzing 'Cython.Build': found module but no type hints or library stubs
pvlib/spa_c_files/setup.py:6: note: See https://mypy.readthedocs.io/en/latest/running_mypy.html#missing-imports
pvlib/solarposition.py:201: error: Skipping analyzing 'pvlib.spa_c_files.spa_py': found module but no type hints or library stubs
pvlib/tests/conftest.py:13: error: "Tuple[str, ...]" has no attribute "base_version"
pvlib/tests/conftest.py:114: error: Skipping analyzing 'pvlib.spa_c_files.spa_py': found module but no type hints or library stubs
pvlib/tests/test_forecast.py:40: error: Need type annotation for '_working_models' (hint: "_working_models: List[<type>] = ...")
pvlib/tests/ivtools/test_sdm.py:9: error: Skipping analyzing 'pvlib.tests.conftest': found module but no type hints or library stubs
pvlib/tests/test_location.py:12: error: Skipping analyzing 'pytz.exceptions': found module but no type hints or library stubs
Found 12 errors in 11 files (checked 83 source files)

@mdeceglie
Copy link
Contributor

mdeceglie commented Feb 12, 2021

FWIW I have been recently introduced to type hints and find that they make the signature much harder to visually parse in, for example, a jupyter tooltip.

@wfvining
Copy link
Contributor

Static analysis as part of the CI pipeline (and for other projects) could be hugely beneficial. I haven't been hugely impressed by mypy so far, particularly since many libraries are not typed, but I think adding type annotations is a great idea. I haven't seen type annotations in jupyter tooltips before, but I would argue that the benefit of static analysis is worth the extra eye strain.

@wholmgren
Copy link
Member Author

Thanks for the feedback on the type annotations. It seems we should probably discuss them in a separate issue, so I will remove them from this PR.

Do you have any feedback on the BaseArray, FixedTiltArray, and SingleAxisArray design?

@kandersolar
Copy link
Member

+1 to discussing type annotations another day.

I initially liked the idea of subclasses inheriting from a BaseArray, but it's unfortunate how much repetition (function signatures, docstrings) is necessary with this inheritance approach. Have we considered composition ("has a") instead of inheritance ("is a")? What I'm imagining is having only one Array class, but it has some sort of mount parameter used like this:

array = Array(mount=FixedMount(20, 180), module=..., temperature_model_parameters=...)

where we have different SomethingMount classes that each provide a mount.get_orientation(solar_zenith, solar_azimuth) method. That way Array doesn't have to know anything about the mount strategy and can just delegate the work to its mount attribute to figure out the tilt and azimuth. Example SomethingMount class definitions:

class FixedMount:
    def __init__(self, surface_tilt, surface_azimuth):
        self.orientation = {
            'surface_tilt': surface_tilt,
            'surface_azimuth': surface_azimuth
        }

    def get_orientation(self, solar_zenith, solar_azimuth):
        return self.orientation  # maybe calculate and return aoi too?


class SingleAxisTrackingMount:
    def __init__(self, axis_tilt, axis_azimuth, max_angle, backtrack, gcr,
                 cross_axis_tilt):
        self.axis_tilt = axis_tilt
        self.axis_azimuth = axis_azimuth
        # and the rest

    def get_orientation(self, solar_zenith, solar_azimuth):
        tracking_data = pvlib.tracking.singleaxis(
            solar_zenith, solar_azimuth,
            self.axis_tilt, self.axis_azimuth,  # and the rest
        )
        return tracking_data

A benefit is that it would be very easy to add new mount strategies (e.g. a DualAxisTrackingMount would be trivial) without a family tree (sea urchin?) of repetitive classes inheriting from BaseArray. Anyway just an idea, I haven't really thought it out beyond what I've written here. If it would help to see it in real code I will work up a draft PR, let me know.

@wfvining
Copy link
Contributor

The idea of a Mount class rather than different Array implementations does sound pretty appealing. It seems like it better models the core feature. I think that it is worth exploring further.

@wholmgren
Copy link
Member Author

@kanderso-nrel that's very interesting and certainly worth exploring in a draft PR. It fits with the rest of the object layer too: ModelChain has a Location and a PVSystem, PVSystem has one or more Array, and now maybe Array has one or more Mount.

I rolled back this PR to its status before I added type annotations. For reference, the annotations are in my types branch.

@cwhanse
Copy link
Member

cwhanse commented Feb 23, 2021

what's the scope of Mount? If it's surface_tilt, surface_azimuth, axis_tilt, axis_azimuth, I'm in favor. We should also consider PVSystem.racking_model which is combined with PVSystem.module_type to look up parameter sets for the SAPM module temperature model. Mount.racking_model + PVSystem.module_type doesn't strike me as an improvement. The current temperature model lookup already seems clumsy.

@wholmgren
Copy link
Member Author

closing in favor of #1176

@wholmgren wholmgren closed this Jun 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make Array play nicely with fixed tilt systems and trackers
5 participants