-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
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. |
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? |
Co-authored-by: RDaxini <[email protected]>
This would be ideal. But when it's just a rename I don't see how it can be deprecated nicely. |
I don't think I agree with this change of name 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:
|
@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.
If I understand it well, then I'm +1 to changing the parameter name and description, +1 to using a self-explanatory parameter name, +1 for |
We already have a definition of pvlib-python/pvlib/pvsystem.py Lines 2309 to 2311 in 4cfda4a
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. |
The De Soto model paper uses the term "absorbed irradiance" for this quantity, symbols |
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 |
+1 |
The distinction between Although I'll point out the irony of this conversation at the same time we are discussing various instances of inconsistent parameter names. |
I would vote for |
(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
addresses the overlap to a sufficient extent so I'll leave my vote neutral (not opposed) on |
There was a problem hiding this 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.
* Changed the `g_poa_effective` parameter name to | ||
`effective_irradiance` in :py:func:`~pvlib.pvsystem.PVSystem.pvwatts_dc`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* 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`. |
@echedey-ls I think I fixed it as per your suggestions - any chance you could take another look? 😄 |
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.
@cwhanse any chance this preference has changed since September? I think yours is the only voice of opposition to 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 |
FWIW the original PVWatts PR does have some discussion about parameter names. #195 (comment) I suspect that many users of |
I'm ok with I concur with the reasoning in #2235 (comment) which, in pvlib terms, is equivalent to In fact, that leads to a possible definition of That's how |
There was a problem hiding this 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
I'm still in favour of using this variable name here, but not convinced about the definition. |
Co-authored-by: Kevin Anderson <[email protected]>
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`
).remote-data
) and Milestone are assigned to the Pull Request and linked Issue.g_poa_effective
is an alternative name foreffective_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.