Stop ModelChain from filling missing DC/AC values with zero (gh-1409)#2822
Closed
adi-IL wants to merge 1 commit into
Closed
Stop ModelChain from filling missing DC/AC values with zero (gh-1409)#2822adi-IL wants to merge 1 commit into
adi-IL wants to merge 1 commit into
Conversation
…1409) ModelChain previously called fillna(0) on the single diode DC results and on the pvwatts AC result. This silently turned genuine missing (NaN) inputs into zero power, which is inconsistent with pvlib's 'NaN in, NaN out' policy and hides data gaps from users. Remove both fillna(0) calls. Nighttime already returns zero power from the single diode model (zero photocurrent), so only genuine gaps are now left as NaN. Add regression tests for the DC (cec) and pvwatts AC paths, and a whatsnew entry.
Member
|
I'm closing this for three reasons, none of which have to do with the improvement that the submitter proposes:
Sorry to be blunt. If you'd like to contribute to pvlib (we welcome that), please discuss the proposed solution to #1409 in the relevant issue, along with evidence that your solution addresses the concern. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes a long standing behavior where
ModelChainsilently replaced missing (NaN) values in its DC and AC results with zero.Background
The single diode path ended with
self.results.dc = tuple(dc.fillna(0) for dc in self.results.dc)and the pvwatts inverter did
self.results.ac = ac.fillna(0)A user who passed genuine gaps (NaN) into the weather therefore received zero power for those time steps instead of NaN, masking the missing data. This is inconsistent with pvlib's documented
NaN in, NaN outpolicy. (See gh-1409.)The masking was originally added so that
ModelChain.results.dc.sum()returned a value at night. We checked that this is no longer needed: at night the single diode model already returns zero power on its own (zero photocurrent gives zerop_mp), so removing the fill leaves nighttime correct while letting true gaps stay NaN.The fix
Remove both
fillna(0)calls. Genuine missing inputs now propagate as NaN through the DC and AC results, and nighttime remains zero power.Testing
test_run_model_preserves_nan_inputs: feeds a three step weather series (day, NaN gap, night) through the cec/single diode DC model and asserts the gap stays NaN, night is zero, and daytime is positive.test_pvwatts_inverter_preserves_nan_inputs: same idea through the pvwatts inverter.tests/test_modelchain.pysuite passes (159 tests).Closes #1409