Repository navigation
Fix cell type inference for First Solar thin-film modules (CdTe, not CIGS) - #2932
Closed
DresdenGman wants to merge 1 commit into
Closed
DresdenGman wants to merge 1 commit into
DresdenGman wants to merge 1 commit into
Conversation
The CEC database lists First Solar modules as "Thin Film", which Array._infer_cell_type maps to CIGS, so ModelChains using the First Solar spectral correction model pick the wrong parameter set for them. Infer "Thin Film" modules as CdTe when the module name identifies them as First Solar modules. First Solar's crystalline silicon (TetraSun) modules are unaffected: their "Mono-c-Si" technology still maps to monosi.
Hey @DresdenGman! 🎉Thanks for opening your first pull request! We appreciate your If AI is used for any portion of this PR, you must vet the content Finally, be sure the PR description includes the PR |
Member
|
This is the wrong path to fix the issue. pvlib has no control over the CEC module data file, so we should not rely on "First Solar" appearing in any field. Thanks for the attempt. I'm closing this PR, discussion of the way forward should be in the Issue. |
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.
docs/sphinx/source/referencefor API changes.docs/sphinx/source/whatsnewfor all changes. Includes link to the GitHub Issue with:issue:1984or this Pull Request with `:pull:`num. Includes contributor name and/or GitHub username (link with :ghuser:DresdenGman).remote-data) and Milestone are assigned to the Pull Request and linked Issue.Fixes #1984.
Array._infer_cell_typemaps Technology "Thin Film" tocigs, but First Solar's thin-film modules are CdTe. Of the 120 First Solar entries in the bundled CEC table, 70 are thin-film modules currently inferred ascigs, which also selects the wrong spectral correction parameters downstream.The change: when the technology is "Thin Film" and the module name identifies the manufacturer as First Solar — a prefix match on the normalized name, checked on both
Array.moduleand themodule_parametersSeries name, since the CEC table has no manufacturer field — the inferred type iscdte. A sweep over the bundled tables confirms the scope: exactly those 70 of 21,535 CEC modules change, none of the 523 Sandia modules change, and First Solar's TetraSun crystalline-silicon modules (Technology "Mono-c-Si") are unaffected.Tests: parametrized cases for both name channels, negative cases for look-alike names, and an integration test using the bundled CEC entry from the issue.
tests/test_pvsystem.py: 330 passed.tests/test_modelchain.py: 160 passed.