Clarify Egref and dEgdT values, changes calcparams_desoto api

[cwhanse]

pvlib python pull request guidelines

Thank you for your contribution to pvlib python!

You may submit a pull request with your code at any stage of completion, however, before the code can be merged the following items must be addressed:

• Closes Clarify module_parameters['EgRef'] usage, and set a default value #462
• Fully tested. Added and/or modified tests to ensure correct behavior for all reasonable inputs. Tests must pass on the TravisCI and Appveyor testing services.
• Code quality and style is sufficient. Passes git diff upstream/master -u -- "*.py" | flake8 --diff and/or landscape.io linting service.
• New code is fully documented. Includes sphinx/numpydoc compliant docstrings and comments in the code where necessary.
• Updates entries to docs/sphinx/source/api.rst for API changes.
• Adds description and name entries in the appropriate docs/sphinx/source/whatsnew file for all changes.

Please don't hesitate to ask for help if you're unsure of how to accomplish any of the above. You may delete all of these instructions except for the list above.

Brief description of the problem and proposed solution (if not already fully described in the issue linked to above):

Sets default values for CEC module parameters Egref and dEgdT in pvsystem.calcparams_desoto. Changes API for pvsystem.calcparams_desoto to individual keyword arguments, removing the module_parameters dict). Adds _build_kwargs to PVSystem.calcparams_desoto

I believe this is ready to merge. I could not fully check out the changes in the tutorial file, since the Jupyter notebook is calling calcparams_desoto

[wholmgren]

Are the changes to the SAM file and irradiance.py due to some git hassles? If so, recommend

git fetch upstream
git checkout upstream/master -- pvlib/data/sam-library-cec-inverters-2018-3-18.csv pvlib/irradiance.py
git commit pvlib/data/sam-library-cec-inverters-2018-3-18.csv pvlib/irradiance.py -m 'checkout master sam and irradiance files'

I'm not 100% sure, but I think this will remove the proposed changes to these files.

[markcampanelli]

I think the desoto model is more naturally parameterized in terms of total irradiance at the POA. I propose that poa_global be renamed to irrad_poa, irrad_ref be renamed to irrad_poa_ref, and temp_ref to temp_cell_ref.

Also, calling this "effective irradiance" might be confused with the G_{eff} appearing in the DeSoto's master's thesis, which appears to have a different definition.

[cwhanse]

@thunderfish24 here we have a conflict between adhering to the published reference (input would be absorbed irradiance, after reflections and soiling losses but without an adjustment from broadband to effective irradiance) and consistency within pvlib-python. SAPM (and PVsyst, when it is implemented #470) accept effective irradiance as input. I would prefer to see Desoto (and CEC, when it is implemented #463) use the same input quantity, with function signature and inline comments documenting the change from literature.

[markcampanelli]

From the standpoint of being able to implement and compare different models easily from the same time-dependent input data (say, global irradiance from pyranometer, albedo, ambient dry bulb temperature, and wind speed), I think having a consistent API would be preferable over complete fidelity to the original model formulation (and documenting the modifications in the code sounds acceptable). It sounds like the definition of "effective irradiance" that you are talking about here is the last one here with SF=1. Can you confirm?

I also realize that my thoughts are muddy on POA irradiance. In particular, the above-mentioned irradiance ratio at the POA doesn't seem to include any of the losses that give DeSoto's S or M factors (or an SF for that matter). It sounds like we plan on moving those factors into a separate function (or functions)?

Computation graphs might be helpful here for the various models taking inputs to outputs.

[cwhanse]

SAPM documentation strikes again. The explanation of effective irradiance in SAPM is rather opaque and is the question we are asked most frequently. A better explanation would be:

• Ee is measured as a ratio of Isc to Isc at reference conditions adjusted for temperature (this is consistent with @thunderfish24 F quantity
• For prediction of power from broadband POA irradiance, Ee is calculated by reducing the POA by surface reflection, soiling and adjusting for spectrum. This calculation is implicit in Eq. 1 on this page : https://pvpmc.sandia.gov/modeling-steps/2-dc-module-iv/point-value-models/sandia-pv-array-performance-model/
  I lost the argument to explicitly present SAPM along these lines on pvpmc.sandia.gov; the majority opinion was to retain the description as in the SAPM report.

From our attempts to bring order and consistency to PV modeling we arrived at this perspective:

• POA irradiance refers to broadband irradiance at the module's face. This is the quantity produced by all of the popular transposition models
• effective irradiance is the irradiance converted to electric current. Not everyone uses this term, but enough papers use it that we decided it made sense to emphasize it.
• translation of POA irradiance to effective irradiance accounts for reflections, soiling, spectral irradiance adjustment, and perhaps other mechanisms.
• De soto (and by inheritance CEC) is the only paper that uses a term for 'adsorbed irradiance', which is POA irradiance less reflections and soiling.

[markcampanelli]

Thanks for the further detailed explanation @cwhanse, which is making better sense to me now. I need a bit more time to think about the "swapping" of this effective irradiance into DeSoto, but it seems reasonable.

Also, it looks like the SAPM's effective irradiance does not try to treat the various angles of incidence of diffuse reflectance on the POA (which seems like a challenging prospect anyhow, and there is an f_D factor).

Note that the "effective irradiance" F in my definition here does not include a temperature correction to Isc0. This is not an approximation, but falls out of the derivation.

[markcampanelli]

One can derive an alternative auxiliary equation for the photocurrent I_L at operating conditions in the Desoto model by setting V=0 in the SDM and rearranging:

I_L = I_sc + I_D * (exp(I_sc * R_s / (N_S * n * V_th(T)) - 1) + I_sc * R_s / R_sh
      = I_sc/I_sc_0 * I_sc_0 + I_D * (exp(I_sc/I_sc_0 * I_sc_0 * R_s / (N_S * n * V_th(T)) - 1) + I_sc/I_sc_0 * I_sc_0 * R_s / R_sh
      = F * I_sc_0 + I_D * (F * I_sc_0 * R_s / (N_S * n * V_th(T)) - 1) + F * I_sc_0 * R_s / R_sh

where some of these other parameters are also computed at operating conditions (e.g., I_D).

One can then use equation (1) in the SAPM as the model for I_sc at operating conditions (possibly also with a SF) to compute F at operating conditions :).

I humbly request that we implement this auxiliary equation for I_L for the SDM in terms of F, which then would then allow us to use either the SAPM or a matched (or unmatched) PV reference device measurement for the determination of the control variable F.

[cwhanse]

@thunderfish24 I could see your request being part of a new function calcparams_(name here). That would be consistent with the API: calcparams_xxxxx returns the 5 values for the single diode equation at irradiance and temperature, etc. My concerns are 1) a source of model parameters (the module-specific constants used by calcparams_xxxxx and 2) ensuring that users understand the input F. Most use cases start with measured broadband irradiance, although pvlib doesn't force the user to consider the instrument behind the measurement.

[markcampanelli]

K. I am happy to implement this after I work out a usage example with the SAPM I_sc in more detail. It seems to me that if someone is already using measured broadband irradiance as a starting point with SAPM, then that workflow should readily adapt to this implementation of SDM with existing calibration parameters (with only a translation of I_L0 to I_sc0, I think).

I also have to first wrap up #426. (I have most of the documentation done there except for some function parameter descriptions and two more tests that I really should write.)

[cwhanse]

@wholmgren yes the changes to the SAM file and irradiance.py are attempts to resolve git conflicts. The checkout/commit sequence you suggested fixed the issue.

[cwhanse]

Ready to merge I think.

[wholmgren]

Mostly looks good.

These changes will break users' code. Do we want to go through the effort of a deprecation cycle? This would probably involve checking if the 4th argument (formerly module_parameters) is a dict or Series, and, if so, dispatching the args/kwargs to an hidden implementation of the old function. The old function would emit a warning. It's not as straightforward as in #427, so maybe not worth the effort.

Inline comments below...

[wholmgren]

Recommend something more like:

pvsystem.calcparams_desoto now requires arguments for each module model parameter.

The arguments you've added are positional, not keyword-only.

[wholmgren]

The whatsnew files generally use an 80 character line limit, though this is not as strict as for python modules.

[wholmgren]

accpted --> accepted

[cwhanse]

shortened to pvsystem.calcparams_desoto now requires arguments for each module model parameter. to stay within character limits.

[wholmgren]

Ok. To be clear and for future reference, you can also continue on to additional lines if you indent them by 2 spaces so that it looks like

* this is a long item....
  that continues here...

[wholmgren]

remove M documentation below here.

[cwhanse]

done

[wholmgren]

soiing --> soiling

[cwhanse]

fixed

[wholmgren]

I will repeat my preference for spelling out most of these arguments in the function signature as in i_from_v. It would require adding some mapping function within PVSystem.calcparams_desoto but I think that's worth it for more descriptive argument names. The descriptive parameter names could be reassigned to the short names to keep the equations more readable.

The abbreviated names are still an improvement to the existing API, so I do not object to moving forward as is after some consideration.

[cwhanse]

I agree with your preference but I'd like to defer it to a pull request with a larger scope. The short argument names, e.g., alpha_sc, R_sh_ref, R_s (notice the discrepancy) are inherited from the column headings in the SAM database file. We read the column headings from the SAM file, do some character replacement, then use the reformatted names as keys.

Using long names in calcparams_desoto would ripple to edits to pvsystem.retrieve_sam, where the long names would be assigned as keys, in order to maintain the functionality of _build_kwargs. I considered doing this but decided it was outside the scope of #462, the issue motivating this pull request.

[wholmgren]

Works for me.

[markcampanelli]

The R_s instead of R_s_ref in SAM has always puzzled me.

[cwhanse]

@wholmgren thanks for the careful checking and proofreading. I have added a less-clumsy exit from calcparams_desoto if the 4th positional argument looks like the old module_parameters. I used warnings.warn - let me know if some other way to convey messages is preferred.

[wholmgren]

I don't know if we really need to bother with a test for the new code path, but did you confirm that it works?

[wholmgren]

I don't understand the grouping of statements here. I would have thought you'd group on the or rather than the and. In any case, I think isinstance(a_ref, (dict, pd.Series)) alone may be what we want. A dict or Series that does not have 'a_ref' will fail in the equations that follow. Better to have it fail in the key look up. But maybe I am just misunderstanding.

[cwhanse]

Tested the if block on my own.

I added that block because I am assuming a use case where multiple sets of module parameters are passed into the new function as pandas.Series objects, and I didn't want the new function to fail in that case.

[wholmgren]

Recommend using warnings.warn(message, DeprecationWarning) where message is both of your statements as one.

[cwhanse]

Done

[wholmgren]

No need for this warning. We already told them once above. Safe to assume success unless exception raised below.

[wholmgren]

Recommend something like

except Exception as e:
    raise e('Module parameters could not be extracted from '
            '4th positional argument. Check that parameters are '
            'from CEC database and/or update your code for new API.')

and then no need for the warning or return below

[cwhanse]

OK

[wholmgren]

I think this is ready to go. Does anyone object?

[cwhanse]

Is there a way to rerun the test? Looks like an unrelated failure in forecast.py

[markcampanelli]

We used to close and reopen PR at my last job, but that was for a different build system.

[mikofski]

@wholmgren or any pvlib admin, can do it from the pvlib Travis ci page there's usually a button that says rebuild. You can also use the Travis API but looks like this build already passed?

[markcampanelli]

C could be confused with Coulombs. Perhaps degC is preferable. (This is also an issue in temp_cell above.)

[wholmgren]

I think the unit is clear in the context of the docstring.

[adriesse]

Amps/Coulomb = 1/second

[cwhanse]

I'd like to leave the temperature unit on the temperature coefficients as C, because that's the unit used in IEC standards and datasheets. I agree with @wholmgren about being clear in context.

[markcampanelli]

nit: Might it be better to say crystalline Silicon here (as opposed to amorphous, etc.)?

[cwhanse]

Good point, 1.121eV is for crystalline silicon.

[markcampanelli]

C could be confused with Coulombs. Perhaps degC is preferable.

[cwhanse]

I changed this one to 1/K, because we use Kelvin in the calculation.

[markcampanelli]

I had some minor suggestions. I noticed some notebook cell execution failures in the desoto section of docs/tutorials/pvsystem.ipynb. Perhaps that notebook needs updating?

[markcampanelli]

Perhaps a comment here on how this differs from the DeSoto model's uses of S, M, etc.?

[cwhanse]

Added

[wholmgren]

I restarted the Appveyor build. I don't know if that requires admin access or not.

[cwhanse]

I updated the notebook docs/tutorials/pvsystem.ipynb but I don't known how to point it at the updated code in this pull request, so it is not fully tested.

[markcampanelli]

Thanks for considering my suggestions.

[wholmgren]

I ran the pvsystem.ipynb tutorial. It looks good to me. Results here. The PR currently contains output with exceptions, but that is ok with me. If you do want to clean it up... I run notebooks against test code using the procedure outlined here. It's simplest if you start the jupyter notebook server within the virtual environment you've created for pvlib development rather than using the jupyter notebook server that came with your base Anaconda installation (assuming that's what you're using). Again, ok with me if we skip it for now.

Thanks @thunderfish24 for your careful reviews.