Repository navigation
[BUG]: modelchain fills nan with 0 #2823
Description
Describe the bug
We fillna(0) twice in modelchain.py
pvlib-python/pvlib/modelchain.py
Line 769 in 9a0ad55
pvlib-python/pvlib/modelchain.py
Line 668 in 9a0ad55
I can open a PR deleting these lines, which would make more sense to me, unless there was a specific reason to do so ?
To Reproduce
def calculate_pv_system_production(
pv_system: SimplifiedPVSystem, solar_inputs: SolarInputs
) -> np.ndarray:
"""
Returns AC power output in kW using the PVWatts model.
"""
# 1. Location
location = pvlib.location.Location(
latitude=pv_system.coordinates.latitude,
longitude=pv_system.coordinates.longitude,
tz=str(pv_system.timezone),
altitude=pv_system.coordinates.altitude,
)
# 2. System
system = pvlib.pvsystem.PVSystem( # todo review all of these parameters with dict
surface_tilt=pv_system.surface_tilt,
surface_azimuth=pv_system.surface_azimuth,
module_parameters={
"pdc0": pv_system.solar_power_kwp * 1000, # W peak DC
"gamma_pdc": pv_system.temp_coefficient,
},
inverter_parameters={
"pdc0": pv_system.solar_power_kwp * 1000,
"eta_inv_nom": pv_system.inverter_efficiency,
},
temperature_model_parameters=pvlib.temperature.TEMPERATURE_MODEL_PARAMETERS[
"sapm"
]["open_rack_glass_glass"],
)
# 3. ModelChain
mc = pvlib.modelchain.ModelChain(
system,
location,
dc_model="pvwatts",
ac_model="pvwatts",
aoi_model="physical",
spectral_model="no_loss",
losses_model="pvwatts",
)
# 4. Run model — pvlib computes sun position internally from location + times
weather_data = {
"dni": solar_inputs.dni_wm2,
"dhi": solar_inputs.dhi_wm2,
"ghi": solar_inputs.ghi_wm2,
}
if solar_inputs.outdoor_air_temp_c is not None:
weather_data["temp_air"] = solar_inputs.outdoor_air_temp_c
solar_inputs_pvlib_format = pd.DataFrame(
index=solar_inputs.ts,
data=weather_data,
)
mc.run_model(solar_inputs_pvlib_format)
# 5. Return AC power in kW
return (mc.results.ac / 1000).to_numpy()Expected behavior
Don't fill NaNs with 0, keep NaNs
Screenshots
This
Becomes
Versions:
pvlib.__version__: 0.15.2pandas.__version__:- python: 3.10
Activity
JoLo90 commentedon Jul 14, 2026
Humm just saw a PR was opened yesterday #2822, and if I would fix it would pretty much look alike
cwhanse commentedon Jul 14, 2026
Overlaps with #1409
@JoLo90 you are already ahead of the game by starting a discussion :)
I suspect that the "set to 0" statements were added to enable lazy summation of energy. Maybe we can add those sums to ModelChainOutput.
My other concern with just removing the set to 0 lines is this: NaNs can be produced by models along the model chain, even from valid weather input. If we remove the set to 0 statements (and I think we should) we may introduce additional NaNs into the output that don't reflect NaN input. Should we distinguish between weather NaN and other-source NaN? I don't know. My reaction is "ugh", because I'm imagining some kind of weather NaN filter being applied to AC output, which may make it inconsistent with the DC stage, etc.
markcampanelli commentedon Jul 14, 2026
For reference, there is numpy.nansum. As a skeptical user, I'd prefer to "opt in" to any usage of this (perhaps through clear disclosure in a docstring). At the cost of interface and implementation complexity, it would be nice to be made aware when such a "workaround" was needed on my particular simulation.
From the discussion in #2808, it sounded like "softer" model failures, such as Perez producing a negative total sky diffuse irradiance on the POA, should be handled differently from merely producing NaN, even though this might signify just as much a "garbage in" situation as a direct NaN input.
Because a pvlib model run can "pass through" or "fail silently" (e.g., not raise on a NaN or otherwise detected bad value), I think at the very least some guidance is warranted in the docs on how users should interpret NaNs (which are fairly easy to work around) vs. the onus to check for other (possibly more subtle) non-sensical results. For example, if a negative POA is buried deep in a ModelChain computation, then is it even detectable in the final energy result? Is it being suggested here that ModelChain play the role of the "savvy user" and perform this layer of additional checking?
JoLo90 commentedon Jul 14, 2026
@markcampanelli @cwhanse
In the example I showed, I would have expected np.nan because, at least to me, nan means "We don't know" while 0 means "There is no pv production".
@markcampanelli can you show me an example where the Perez model "produce a negative total sky diffuse irradiance on the POA" ? Or some other examples where we rather pass NaN instead of negative values ?
markcampanelli commentedon Jul 15, 2026
@JoLo90 Here are morning and afternoon examples I generated from an NSRDB TMY file, which had negative horizon diffuse in 516 of 8760 timestamps. (I think this phenomenon is called horizon darkening.) My understanding is that this is "correct" Perez model behavior. However, one can also choose inputs that produce very questionable negative diffuse components (and not just in the horizon).
UPDATE: I forgot to include that
surface_tilt = 20.0andsurface_azimuth = 180.0.JoLo90 commentedon Jul 21, 2026
@markcampanelli thanks for the example.
I would say that as long as it makes mathematical and physical sense (if pow_horizon is negative because it lowers the poa_sky_diffuse and thus makes its calculation more precise) we could keep negative values.
I think it is not problematic as long as it's not the the "final" output (ex: negative DC/AC power doesn't make sense, if it is due to numerical instability clip to 0 would be my approach). In a way, we decide for the user what the correct value is, and we don't do GIGO anymore, but I think this is domain knowledge of the pvlib maintainers and the added value of the pvlib library. The problem with this issue is that we don't respect the nan in -> nan out philosophy anymore.
varma1221 commentedon Oct 3, 2026
Hello @cwhanse @JoLo90
Since #2822 was closed pending agreement here, I removed both
fillna(0)calls on main to see where NaN shows up from valid weather, which was @cwhanse's concern. I found four cases:1. Trackers at night:
SingleAxisTrackerMount.get_orientationreturns NaN tilt when the sun is down, so every night hour becomes NaN for every DC model (4358 h in the bundled TMY3). ModelChain used to stow ataxis_tiltin_prep_inputs_tracking, but that hasn't been called since #1771. If I put the stow back, the NaNs go away and nothing else changes.2.
spectral_model='first_solar'at night: Airmass is NaN for zenith > 90, so the modifier is NaN even when POA is 0. Checking forpoa == 0isn't enough: 297 TMY3 hours just after sunset still have POA > 0 because of hourly averaging. Using a modifier of 0 where airmass is NaN, asspectral_factor_sapmalready does, fixes it, and annual energy is the same as on main.3. Negative night irradiance: 210 rows in the SURFRAD test file give negative I_L and R_sh, and cec/desoto/pvsyst return NaN.
4. Tiny positive irradiance (#1673), e.g. E = 1e-15 W/m² with cec.
Both 3 and 4 go away if effective irradiance below 1e-9 is set to 0 before calcparams, as suggested in #1673. NaN still passes through. With that change and the first_solar fix, I compared 540 outputs against main (TMY3 and SURFRAD,ccec/desoto/pvsyst, all AOI and spectral models, single and multi-array). Every value that is valid in both matches, apart from one lambertw case mentioned at the end. The existing modelchain tests also pass without edits.
A few questions before I open a PR:
spectral_factor_firstsolar?ModelChainOutput: in this PR or a follow-up?Separately,
singlediodecan still return NaN whenv_occomes out around 1e-26 instead of exactly 0. #2862 doesn't cover that, so I'll open a separate issue for it.Happy to get assigned and open the PR once we agree on a direction
JoLo90 commentedon Oct 6, 2026
@varma1221 thanks for following up
1. Trackers at night: I am wondering if it wouldn't be easier to avoid filtering for sun above panel horizon and simply pass the returned values at night. Removing the checks returns a vertical surface at night see below.