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

Change g_poa_effective to effective_irradiance #2235

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

Conversation

AdamRJensen
Copy link
Member

@AdamRJensen AdamRJensen commented Sep 29, 2024

  • Related to variables_style_rules.csv could use an update #1253
  • I am familiar with the contributing guidelines
  • Tests added
  • 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 (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

g_poa_effective is an alternative name for effective_irradiance. The latter is the standard in pvlib (at least the most widely used).

This is a breaking change, but I don't see how this can be deprecated in a nice way.

The previous definition was also not correct. IMO it doesn't you can't use the term "broadband" about effective irradiance which has potentially been modified for spectral effects.

g_poa_effective;broadband plane of array effective irradiance.

@AdamRJensen AdamRJensen marked this pull request as ready for review September 29, 2024 20:56
@echedey-ls
Copy link
Contributor

Should a deprecation-warning period be established? I'm not aware of the internal methodology you guys make use of, but I've seen that changing params have been deprecated before in this project. See #773.

@RDaxini
Copy link
Contributor

RDaxini commented Sep 29, 2024

The previous definition was also not correct. IMO it doesn't you can't use the term "broadband" about effective irradiance which has potentially been modified for spectral effects.

g_poa_effective;broadband plane of array effective irradiance.

If you apply a dimensionless spectral factor correction to a value of broadband irradiance in Wm⁻², it's still a broadband value in Wm⁻² but just a fraction of the original broadband value. Broadband just means its the irradiance between a (wide) wavelength range, no?

@AdamRJensen
Copy link
Member Author

AdamRJensen commented Sep 29, 2024

Should a deprecation-warning period be established? I'm not aware of the internal methodology you guys make use of, but I've seen that changing params have been deprecated before in this project. See #773.

This would be ideal. But when it's just a rename I don't see how it can be deprecated nicely.

@cwhanse
Copy link
Member

cwhanse commented Sep 29, 2024

I don't think I agree with this change of name g_poa_effective to effective_irradiance. The current description is of a quantity that is not poa_global but also is not effective_irradiance which is supposed to be after accounting for reflections, soiling, and spectrum. I agree that the description can be improved.

Just a comment about names: sometimes variable names were chosen to more closely connect the code with the text in the reference. Didn't happen here (the reference uses :math:I_{tr} so I'm guessing the submitter of this code came up with g_poa_effective as a compromise :math:G_{POA} being common in literature, and adding "_effective" because reflections have been removed.

    g_poa_effective: numeric
    effective_irradiance: numeric
        Irradiance transmitted to the PV cells. To be
        fully consistent with PVWatts, the user must have already
        applied angle of incidence losses, but not soiling, spectral,
        etc. [W/m^2]

@echedey-ls
Copy link
Contributor

echedey-ls commented Sep 29, 2024

This would be ideal. But when it's just a rename I don't see how it can be deprecated nicely.

@AdamRJensen , we can work it out, similarly to #773 with some little variations. Hint: signature will need to be:

def pvwatts_dc(self, effective_irradiance=None, temp_cell=None, *, g_poa_effective=None):

and the warning when it's used:

if g_poa_effective:
    warnings.warn("Use 'effective_irradiance' to suppress this warning", pvlibDeprecationWarning)
    effective_irradiance=g_poa_effective
    tests of mutual exclusion

then the warning test.

because reflections have been removed

If I understand it well, then [post_]iam_irradiance, iam_modified_irradiance, irradiance_after_iam, poa_global_after_iam, poa_g_after_iam may be options that fit the description better. What do you think about it @cwhanse ?

I'm +1 to changing the parameter name and description, +1 to using a self-explanatory parameter name, +1 for poa_global_after_iam if I had to choose, not a strong opinion.

@RDaxini
Copy link
Contributor

RDaxini commented Sep 29, 2024

The current description is of a quantity that is not poa_global but also is not effective_irradiance which is supposed to be after accounting for reflections, soiling, and spectrum. I agree that the description can be improved.

We already have a definition of effective_irradiance, which includes both reflection and spectral effects:

pvlib-python/pvlib/pvsystem.py

Lines 2309 to 2311 in 4cfda4a

effective_irradiance : numeric
Effective irradiance accounting for reflections and spectral content.
[W/m2]

So the definition proposed here (AOI correction only) would conflict with the existing definition above.

I was just reading through the pvwatts manual now --- page 9 eqn 8 --- perhaps we could just just use the same "transmitted" terminology here instead, i.e. transmitted_irradiance?

@cwhanse
Copy link
Member

cwhanse commented Sep 29, 2024

"transmitted" terminology here instead, i.e. transmitted_irradiance?

The De Soto model paper uses the term "absorbed irradiance" for this quantity, symbols $S$ and $S_{ref}$

@kandersolar
Copy link
Member

kandersolar commented Sep 30, 2024

If this parameter name changes, we should consider doing it in combination with broader changes to that function (name and module, see #1350 and #1544 (comment)).

PVWatts v5 doesn't account for spectrum (so you could say implicitly $M=1$). Soiling is applied as a generic loss factor after DC power is modeled. I agree that effective_irradiance is not quite right for the quantity in question, but that bothers me less than inventing/adopting a new term would. I suggest renaming to effective_irradiance and listing the caveats in the parameter description.

@AdamRJensen
Copy link
Member Author

If this parameter name changes, we should consider doing it in combination with broader changes to that function (name and module, see #1350 and #1544 (comment)).

PVWatts v5 doesn't account for spectrum (so you could say implicitly M = 1 ). Soiling is applied as a generic loss factor after DC power is modeled. I agree that effective_irradiance is not quite right for the quantity in question, but that bothers me less than inventing/adopting a new term would. I suggest renaming to effective_irradiance and listing the caveats in the parameter description.

+1

@cwhanse
Copy link
Member

cwhanse commented Sep 30, 2024

I suggest renaming to effective_irradiance and listing the caveats in the parameter description.

The distinction between poa_global and effective_irradiance is probably not important to typical users of pvwatts_dc. If we are choosing to rename g_poa_effective to one of the already-used terms, I would prefer poa_global rather than effective_irradiance.

Although I'll point out the irony of this conversation at the same time we are discussing various instances of inconsistent parameter names.

@adriesse
Copy link
Member

adriesse commented Oct 1, 2024

I would vote for effective_irradiance because that's consistent with the other DC module models/functions.

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.

6 participants