Skip to content

Inconsistent parameter name in fit_sdm_cec_sam()? #958

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

Open
Peque opened this issue Apr 25, 2020 · 3 comments
Open

Inconsistent parameter name in fit_sdm_cec_sam()? #958

Peque opened this issue Apr 25, 2020 · 3 comments
Labels
Milestone

Comments

@Peque
Copy link
Contributor

Peque commented Apr 25, 2020

The parameter beta_voc is named after v_oc.

The parameter alpha_sc is a bit inconsistent with this, since it should be alpha_isc (after i_sc).

In fact, the underlying SAM function also expects that name:

datadict = {'tech_model': '6parsolve', 'financial_model': None,
'celltype': celltype, 'Vmp': v_mp,
'Imp': i_mp, 'Voc': v_oc, 'Isc': i_sc, 'alpha_isc': alpha_sc,
'beta_voc': beta_voc, 'gamma_pmp': gamma_pmp,
'Nser': cells_in_series, 'Tref': temp_ref}

@Peque
Copy link
Contributor Author

Peque commented Apr 25, 2020

I can push a MR if you want. Feel free to close this as well. I understand it is not too bad as-is and it breaks backwards compatibility. 😊

@cwhanse
Copy link
Member

cwhanse commented Apr 25, 2020

This is probably a legacy of PVLib Matlab, where alpha_isc has units of 1/C, and alpha_sc has units of A/C. Let's hold off a PR for now. I think we will rename a batch of arguments for v0.8, but I don't think we're agreed on the new names.

@CameronTStark CameronTStark added this to the 0.7.3 milestone Apr 27, 2020
@wholmgren wholmgren modified the milestones: 0.7.3, 0.8.0 Jul 17, 2020
@wholmgren wholmgren modified the milestones: 0.8.0, 0.9.0 Aug 28, 2020
@Peque
Copy link
Contributor Author

Peque commented Dec 7, 2020

Just a note: having alpha in A/ºC and beta in V/ºC but gamma in %/ºC can cause trouble/confusion.

Also, in my probably biased and reduced experience, datasheets usually have these 3 values in %/ºC, maybe it could be interesting to consider a change to percentage as well.

@cwhanse cwhanse mentioned this issue May 5, 2021
24 tasks
@wholmgren wholmgren modified the milestones: 0.9.0, 0.10.0 Aug 7, 2021
@kandersolar kandersolar modified the milestones: 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
Labels
Projects
None yet
Development

No branches or pull requests

5 participants