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

Accept float in PVSystem.get_irradiance #2227

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

cwhanse
Copy link
Member

@cwhanse cwhanse commented Sep 26, 2024

If solar_zenith is not a Series with a DatetimeIndex, then the solar constant is used.

@cwhanse
Copy link
Member Author

cwhanse commented Sep 26, 2024

Working on this bug fix uncovers a related bug: the pvsystem.PVSystem.get_irradiance docstring says that irradiance parameters can be tuple of float, but that doesn't work:

system = pvsystem.PVSystem(surface_tilt=32, surface_azimuth=135)
irrads = {'dni': (900., 0.), 'ghi': (600., 0.), 'dhi': (100., 0.)}
zenith = 55.366831
azimuth = 172.320038
irradiance = system.get_irradiance(zenith,
                                   azimuth,
                                   irrads['dni'],
                                   irrads['ghi'],
                                   irrads['dhi'])

raises ValueError: Length mismatch for per-array parameter

So tuple of float of length N only works with a PVSystem with N Arrays.

I would like to edit the docstring to remove the promise that a tuple of floats will work. If more than one irradiance value is to be processed, they can be supplied in a Series. A tuple of Series, one Series for each Array, should still be accepted.

@cwhanse cwhanse marked this pull request as ready for review September 27, 2024 18:17
@kandersolar kandersolar added this to the v0.11.2 milestone Sep 27, 2024
@kandersolar
Copy link
Member

So tuple of float of length N only works with a PVSystem with N Arrays.

Isn't that the intended behavior? Tuples for these inputs are supposed to index over Arrays. That example:

irrads = {'dni': (900., 0.), 'ghi': (600., 0.), 'dhi': (100., 0.)}
irradiance = system.get_irradiance(..., irrads['dni'], irrads['ghi'], irrads['dhi'])

is simulating a single timestamp for system with two Arrays, where for some reason it is daytime for one Array and night for the other. The specific values are not realistic, but the use case is still valid (IMHO). I wouldn't call it a bug.

@cwhanse
Copy link
Member Author

cwhanse commented Sep 27, 2024

It would be simpler to describe behavior if we could state "tuples are distributed over Arrays". But that's not what happens when the system has only one Array: the tuple is treated the same as a Series of timestamped values.

If we want to keep the case of "tuple of float" I can put that back in the docstring as:

dni: float, tuple of float, Series or tuple of Series
    Direct normal irradiance [W/m2]


Notes
------
        Each of ``dni``, ``ghi``, and ``dni`` may be passed as a float, tuple of
        float, Series or tuple of Series. If passed as a float or Series, these
        values are used for every Array. If passed as a tuple of float and there
        is more than one Array, the tuple is distributed over the Arrays so the
        tuple length must be the number of Arrays. If passed as a tuple of Series
        the tuple is distributed over the Arrays and the tuple length must be
        the number of Arrays.

@kandersolar
Copy link
Member

It would be simpler to describe behavior if we could state "tuples are distributed over Arrays". But that's not what happens when the system has only one Array: the tuple is treated the same as a Series of timestamped values.

Can you provide an example? I'm not seeing that behavior:

from pvlib.pvsystem import PVSystem
import pandas as pd

system = PVSystem(surface_tilt=32, surface_azimuth=135)

for converter in [pd.Series, tuple]:
    irradiance = system.get_irradiance(
        solar_zenith=converter([45, 45]),
        solar_azimuth=converter([180, 180]),
        dni=converter([900, 900]),
        ghi=converter([600, 600]),
        dhi=converter([100, 100])
    )
    print(irradiance)

@cwhanse
Copy link
Member Author

cwhanse commented Sep 27, 2024

Can you provide an example? I'm not seeing that behavior.

Neither am I. I must have been imagining what was originally intended by including "tuple of float" in the docstring.

To be sure we're seeing the same thing, that code produces ValueError: Length mismatch for per-array parameter for converter=tuple. Correct?

And this is the case we want to accomodate?

@kandersolar
Copy link
Member

To be sure we're seeing the same thing, that code produces ValueError: Length mismatch for per-array parameter for converter=tuple. Correct?

Correct.

And this is the case we want to accomodate?

On the contrary, I think pvlib already behaves correctly. Tuples should get distributed over Arrays, so tuples of anything (float or Series) should only work when the length of the tuple equals the number of Arrays. The case above shows that a single-Array PVSystem raises an error when given tuples of length 2. That seems correct to me. So what I am saying is that I think the "related bug" described in #2227 (comment) is actually the intended behavior.

@cwhanse
Copy link
Member Author

cwhanse commented Sep 27, 2024

Tuples should get distributed over Arrays, so tuples of anything (float or Series) should only work when the length of the tuple equals the number of Arrays.

We are in agreement now. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pvsystem.Array.get_irradiance raises an error if solar_zenith is passed as a float
2 participants