Fix noct_sam()
@cwhanse
- [x] Closes #1462
- [x] I am familiar with the contributing guidelines
- [x] Tests added
- [ ] Updates entries in docs reference
- [x] Adds description to what's new
- [x] New code is fully documented .
- [x] Pull request is nearly complete and ready for detailed review.
The problem was that 'noct_sam' did not correctly account for the 'effective_irradiance' parameter. The original implementation always used 'poa_global' when computing the initial temperature rise, which caused:
- Open-circuit modules (efficiency = 0) to ignore 'effective_irradiance'
- Very low 'effective_irradiance' values to produce unrealistic cooling
- Results inconsistent with SAM under certain option combinations
This PR updates 'noct_sam' so that:
- 'effective_irradiance' is used consistently when provided
- Negative or very small irradiance values are clipped to zero
- Computation now matches SAM’s documented behaviour
- I updated the noct_sam function to correctly incorporate effective irradiance into the temperature model.
New tests were added to validate:
- Temperature decreases with reduced effective irradiance
- Open-circuit modules still respond to effective irradiance changes
- Temperature never falls below ambient for extremely small irradiance
These tests pass under the main branch, confirming correct behaviour.
@cwhanse Kindly review the updated implementation.
@Chirag3841 please merge the current main, this PR is missing the changes made in #2605
I don't think we want to patch noct_sam this way. My thought is that we replace poa_global with effective_irradiance in the first arg position, drop effective_irradiance=None, and do away with the POA ratio internally. That matches the SAM calculation. The SAM function also handles calculation of effective irradiance, which isn't the job of this function in pvlib.
Replacing the argument requires a deprecation. @Chirag3841 if the deprecation machinery is a bit much, OK to close this PR and thanks for attempting the fix.
@cwhanse I tried the approach where effective_irradiance=None defaults to poa_global and emits a deprecation warning otherwise. However, this caused ambiguity in the function signature and resulted in multiple failures across ModelChain, PVSystem, and tests due to mixed positional and keyword arguments. I now understand that keeping both poa_global and effective_irradiance in the same function is not a good approach. I will revert this logic and instead keep noct_sam aligned with SAM by using effective_irradiance as the first argument only, and handle backward compatibility via a separate deprecated wrapper function.