Skip to content

Change g_poa_effective to effective_irradiance #2235

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

Merged
merged 14 commits into from
Jun 4, 2025

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.

@RDaxini
Copy link
Contributor

RDaxini commented Oct 8, 2024

(not sure if my earlier comment was holding this up so, just in case, a follow up comment...)

In general I am not keen on reusing an existing variable with a different definition (comment), but in this specific case I think @kandersolar's comment

(so you could say implicitly M = 1 )

addresses the overlap to a sufficient extent so I'll leave my vote neutral (not opposed) on effective_irradiance

@cwhanse cwhanse added this to the v0.12.0 milestone Feb 27, 2025
@cwhanse cwhanse added the deprecation Use for issues and PRs which involve deprecations label Feb 27, 2025
@kandersolar kandersolar modified the milestones: v0.12.0, v0.11.3 Mar 14, 2025
Copy link
Contributor

@echedey-ls echedey-ls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nitpicking on the readme and proposal of using the may-be-keyword argument decorator to give some graceful warnings. Seems a good change to me.

Comment on lines 44 to 45
* Changed the `g_poa_effective` parameter name to
`effective_irradiance` in :py:func:`~pvlib.pvsystem.PVSystem.pvwatts_dc`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Changed the `g_poa_effective` parameter name to
`effective_irradiance` in :py:func:`~pvlib.pvsystem.PVSystem.pvwatts_dc`.
* Rename parameter name ``g_poa_effective`` to ``effective_irradiance`` in
:py:func:`~pvlib.pvsystem.PVSystem.pvwatts_dc` and :py:func:`~pvlib.pvsystem.pvwatts_dc`.

@kandersolar kandersolar modified the milestones: v0.12.0, v0.13.0 Mar 19, 2025
@AdamRJensen
Copy link
Member Author

@echedey-ls I think I fixed it as per your suggestions - any chance you could take another look? 😄

@kandersolar
Copy link
Member

Whatever change we make here, it would be nice (though not required, since we're just deprecating, not removing) to make it in the .0 release this week.

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.

@cwhanse any chance this preference has changed since September? I think yours is the only voice of opposition to effective_irradiance.

FWIW I still think the reasoning in #2235 (comment) is sound; PVWatts v5 accounts for soiling elsewhere, and spectrum not at all, so IMHO the irradiance quantity in question is effective_irradiance in spirit as far as PVWatts v5 is concerned. I don't think calling it poa_global is suitable given that it's supposed to account for IAM.

@wholmgren
Copy link
Member

FWIW the original PVWatts PR does have some discussion about parameter names. #195 (comment)

I suspect that many users of pvwatts_dc are applying soiling or spectral losses before calling this function. So my vote in 2025 is to call it effective_irradiance, note the discrepancy with the literature, and move on. I don't think we want to introduce something like the originally proposed irradiance_transmitted or variations.

@cwhanse
Copy link
Member

cwhanse commented Jun 4, 2025

any chance this preference has changed since September? I think yours is the only voice of opposition to effective_irradiance.

I'm ok with effective_irradiance.

I concur with the reasoning in #2235 (comment) which, in pvlib terms, is equivalent to pvwatts_dc setting spectrum_model='no_loss'. From this perspective the input is effective_irradiance.

In fact, that leads to a possible definition of effective_irradiance: broadband plane-of-array irradiance reduced by reflections, soiling and adjusted for spectrum.

That's how effective_irradiance is usually computed for PV modeling. In the context of model calibration from outdoor measurements, effective_irradiance is different: the irradiance estimated from measured Isc.

Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AdamRJensen there's a g_poa_effective in the new agriPV example too

@adriesse
Copy link
Member

adriesse commented Jun 4, 2025

In fact, that leads to a possible definition of effective_irradiance: broadband plane-of-array irradiance reduced by reflections, soiling and adjusted for spectrum.

I'm still in favour of using this variable name here, but not convinced about the definition.

@AdamRJensen AdamRJensen merged commit 3bc67a2 into pvlib:main Jun 4, 2025
30 checks passed
@AdamRJensen AdamRJensen deleted the remove_g_poa_effective branch June 4, 2025 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api deprecation Use for issues and PRs which involve deprecations documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants