Repository navigation
Do not infer First Solar spectral coefficients for 'Thin Film' modules - #2940
Open
chenyi6758-source wants to merge 3 commits into
Open
chenyi6758-source wants to merge 3 commits into
chenyi6758-source wants to merge 3 commits into
Conversation
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.
Closes #1984
CEC Technology 'Thin Film' is ambiguous -- it may be CdTe, CIGS, or another thin-film type -- but _infer_cell_type maps it to CIGS, so First Solar CdTe modules in the CEC database (e.g. FS_4117_3) silently got CIGS spectral coefficients. At pw=3, AMa=3 the modifier comes out 1.0297 instead of 1.0188 for CdTe.
This follows the direction cwhanse set out on #1984 (Nov 3): do not infer coefficients in this case. PVSystem.first_solar_spectral_loss now raises a ValueError telling the user to set Technology to the actual cell type (e.g. 'CdTe') or supply first_solar_spectral_coefficients explicitly. (The earlier PR #2932 took a name-based approach instead; this PR does not do that.)
Local verification on upstream main c5de586: the FS_4117_3 case now raises ValueError; Technology='CdTe' gives the CdTe modifier and user-supplied coefficients are used as-is; tests/test_pvsystem.py 303 passed, 13 skipped; modelchain spectral tests 9 passed; spectrum mismatch tests 56 passed.