Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update CEC module and inverter tables with latest SAM files #1433

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

kandersolar
Copy link
Member

  • Closes CEC module library is behind current version in SAM #1345
  • I am familiar with the contributing guidelines
  • Tests added
  • [ ] Updates entries in docs/sphinx/source/reference for API changes.
  • Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for all changes. Includes link to the GitHub Issue with :issue:`num` or this Pull Request with :pull:`num`. Includes contributor name and/or GitHub username (link with :ghuser:`user`).
  • [ ] New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels and Milestone are assigned to the Pull Request and linked Issue.

It's a little concerning how many rows got dropped in each table (compare the line counts in the diff). A very brief and non-comprehensive comparison suggests that the upstream CEC files did indeed drop a lot of rows, so I guess it is what it is.

@kandersolar kandersolar mentioned this pull request Mar 24, 2022
14 tasks
@kandersolar
Copy link
Member Author

Not sure why the checks aren't showing up for the latest commit; github's webhooks have been having issues this week, but githubstatus.com doesn't show anything amiss at the moment. Closing and reopening...

@kandersolar kandersolar reopened this Mar 24, 2022
@kandersolar
Copy link
Member Author

I guess this is ready for review. I'm still very confused about the missing rows. For example there's not a single Canadian Solar module in either of these two tables on the CEC website, so they're not in these SAM files either. Am I missing something?

@adriesse
Copy link
Member

Could we have a file that is the union of the current and older files? Perhaps with a date or version label added?

@kandersolar
Copy link
Member Author

A merged file seems like the best outcome, but it might be difficult to achieve. See #761 (comment) and https://sam.nrel.gov/forum/forum-general/3779-missing-pv-modules-and-inverters.html

When they open in a few hours I will call the CEC to ask about these removals; maybe there is some mistake.

@cwhanse
Copy link
Member

cwhanse commented Mar 25, 2022

The CEC has been removing equipment from their list when certifications expire. https://www.energy.ca.gov/files/equipment-noticed-removal

Some time ago, I had a conversation wtih SAM developers about retaining obsolete listings. The compromise was to retain old versions of the SAM files but I've lost track of that location.

@adriesse
Copy link
Member

I doubt it's technically difficult. Alternatively pvlib can just add rather than replace new versions of the files.

@cwhanse
Copy link
Member

cwhanse commented Mar 25, 2022

I doubt it's technically difficult.

It would seem not, but in fact is an onerous task to merge these files. The NREL process relies on the CEC list's manufacturer and equipment identifiers to assign a key, and CEC changes the format of those fields every few years. To make matters worse, submitters provide the values so when a company or brand changes hands, the module identifier may or may not change. The result is the same module with different keys; to avoid duplicate or competing entries, one would have to merge these lines manually.

Alternatively pvlib can just add rather than replace new versions of the files.

Yes, at the risk of getting user questions about different parameter sets and record structures.

I am convinced the long-term solution is to build out pvlib/pvmodules to have public code that generates the parameters, and to curate the inputs by frequently pulling the CEC list, identifying new items, and adding the content to a pvmodules database, thus accumulating an archival list in a common format.

@cwhanse
Copy link
Member

cwhanse commented Mar 25, 2022

To make my view clear, @adriesse suggested "Alternatively pvlib can just add rather than replace new versions of the files."

For this PR, I agree with this suggestion.

@kandersolar
Copy link
Member Author

pvsystem.retrieve_sam currently has no concept of different versions of the same table. Would we add dates to the accepted name values, e.g. retrive_sam(name='cecmod-2021-12-01')? Would name='cecmod' point to the new or the old table?

@mikofski
Copy link
Member

mikofski commented Mar 25, 2022

this might break existing code, but my preference would be to reference the file by name. Maybe not even ship it with pvlib, but tell folks you can retrieve from NREL/SAM GitHub repo: NREL/SAM/deply/lbraries/CEC_Modules.csv

For example:

  1. download the CSV from NREL/SAM gh repo
  2. retrieve_sam(file='path/to/locally/downloaded/CEC_Modules.csv')

Are there missing steps, EG: is the SAM file pre-processed somehow for pvlib?

Another option, would be to leverage the Version column in CEC_Modules.csv and just merge or append the lists together, and force the user to select the version they want as a separate parameter:

retrieve_sam(name='cecmod', version='SAM 2021.12.02')  # use versions=None for latest

there's also a Date field

@cwhanse
Copy link
Member

cwhanse commented Mar 25, 2022

tell folks you can retrieve from NREL/SAM GitHub repo

The issue here is that SAM decided to replace the file at each SAM release, so the latest version has the missing/dropped module problem. The archive of parameters for modules dropped by the CEC is the set of files for previous SAM versions.

leverage the Version column

that's a good idea.

@kandersolar
Copy link
Member Author

kandersolar commented Mar 25, 2022

I've added a version parameter to switch between the two versions. I've kept the version data files separate instead of stacking them into one table, in part because they don't have exactly the same columns and in part because I just thought it was cleaner. Is this more or less what folks were thinking or am I barking up the wrong tree?

Edit: I couldn't get the CEC office on the phone so I emailed them instead. No reply yet.

@wholmgren
Copy link
Member

wholmgren commented Mar 25, 2022

Personally I would recommend posting those files somewhere online (zenodo?), removing them from the distribution, and forcing users to manually specify a file path. (I think this is similar to Mark's first suggestion). I'm not too thrilled with complicating the pvlib api for file access. But no worries if you want to go this way.

@mikofski
Copy link
Member

SAM decided to replace the file at each SAM release

I believe I can crawl their GitHub history to retrieve the older files

the long-term solution is to build out pvlib/pvmodules to have public code that generates the parameters, and to curate the inputs by frequently pulling the CEC list, identifying new items, and adding the content to a pvmodules database

That’s half of what pvfree already does. If only I had more time to work on it. And I feel your pain, merging and parsing constantly shifting model & mfg names is why I made the database to begin with

@kandersolar kandersolar modified the milestones: 0.9.1, 0.9.2 Mar 29, 2022
@kandersolar kandersolar modified the milestones: 0.9.2, 0.9.3 Aug 19, 2022
@kandersolar kandersolar modified the milestones: 0.9.3, 0.9.4 Sep 14, 2022
@kandersolar kandersolar mentioned this pull request Dec 9, 2022
9 tasks
@kandersolar kandersolar modified the milestones: 0.9.4, 0.9.5 Dec 14, 2022
@kandersolar kandersolar modified the milestones: 0.9.5, 0.9.6 Mar 18, 2023
@kandersolar kandersolar modified the milestones: 0.9.6, 0.10.0, Someday May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CEC module library is behind current version in SAM
5 participants