Fix clearsky Ineichen rounding #808
Conversation
* handle warnings in temperature model tests * test for content of exceptions * test warning message content * fix it up * correct indent on asserts * fix indent on pop * fix exceptioninfo reference * use match kwarg * use match kwarg throughout
…b#797) * replace Pandas item() implementation with numpy's using .values * force value to np.ndarray to enable universal use of .item()
* mark xfail of test_get_psm3 * add line for lint fix
* function getparams_desoto added to pvsystem module. This commit is just the copy from my previous work, slighty modified for removing the PEP8 warnings. * getparams_desoto moved from pvsystem to singlediode. - getparams_desoto renamed in getparams_from_specs - some initializing values were changed in accordance with Duffie&Beckman2013 * - Modification of getparams_from_specs so as it follows better the procedure of DeSoto&al(2006). * - Bug corrected in DeSoto(2006) procedure - test_singlediode completed with a beginning of test_getparams_from_specs * - test_getparams_from_specs_desoto() finished - function renamed as above * Not sure of all changes brought by this commit because of holidays. - function 'from_epw' added in location - 1 modification in singlediode - 1 modification in test_singlediode * - function '_parse_raw_sam_df' modified. The parser engine is now defined on 'python'. If not the pd.read_csv cannot work with me. * - ModelChain attribute 'diode_params' transformed from tuple containing pd.Series to DataFrame. Makes the use of diode_params easier for further calculations. * - singlediode.get_params_from_specs_desoto() output changed. 'a_ref' is changed by 'nNsVth_ref' * read_epw changed. A line has been added to convert the precipitable water from mm to cm, in order to be compatible with other functions of pvlib * - read_epw changed. If condition added to make the conversion only in the case of TMY3 * - argument diode_params changed from tuple to pd.DataFrame * - get_params_from_specs_desoto removed from singlediode.py * - function fit_sdm_desoto added. Still need to be formatted * - change on type of self.diode_params removed. Go check on branch diode_params_in_df for seeing it * - Function 'fit_sdm_desoto' cleaned and variables names named as in 'fit_sdm_cec_sam' * - all changes made on other files than ivtools.py removed (cleaning for comparing before PR) * - other differences cleaned * - renaming of one variable and minor documentation modifications * - Beginning of writting of test_fit_sdm_desoto. Coverage around 90-95% I think * -minor format changes * - changes made according to feedbacks of markcampanelli * - some cleaning on fit_sdm_desoto to make it more readable - tests completed * - minor code cleaning * - check on importation of scipy removed * - minor cleaning * - attempt to reach 100% coverage * - description added in docs/sphinx/source/whatsnew/v0.7.0.rst * - changes made according to cwhanse review. Except alpha_sc and beta_voc still in %/K * - minor correction and adaptation of test * - sign correction on 3rd equation * - changes on units of alpha_sc and beta_voc inputs. Now in A/K and V/K rather than %/K. - 'celltype' input replaced by EgRef and dEgdT, with values of Si as default * - other line of test added for better coverage * - some changes to try feedbacks of cwhanse and markcampanelli, not finished * -OptimizeResult added in output - solver_method removed - docstring modified - result.message included in message raised by RuntimeError * - includes all feedbacks made on the 21/10, except moving of pv_fct() to the module level * - pv_fct moved out of the fit_sdm_desoto function and renamed in _system_of_equations * - minor modification: Boltzmann k given in specs to avoid import of scipy in _system_of_equations * - cleaning and minor modifications to docstring * - references added to docstring in _system_of_equations * - modification for removing last lint error * - modification for removing last lint error * - modification for removing lint error * - integration of adriesse suggestions * - adding of tylunel to the list of contributors * - adding of mark requires_scipy
…builtin input function (pvlib#800) * replace Pandas item() implementation with numpy's using .values * force value to np.ndarray to enable universal use of .item() * rename function parameters to arg instead of input * convert last input to arg * remove extra lines
* Functional new function. * Undo * Working function; partial docstring; no tests. * Complete docstring; simplify a bit; start tests. * Getting better all the time! * Adjust some comments and update whatsnew. * Improvements as per review. * More changes as requested. * Fun with stickler. * Docstring corrections. * Remove all controversial code.
|
Agree we should explain the connection between code and sources. I'm having trouble finding the value 0.16268 in the Perez paper [2] - can you point me to where you found it? |
|
[2] page 312: 0.83 * 0.196 = 0.16268 [1]:
|
|
Looks like a bunch of unrelated commits popped up in this PR, might not be ready to merge as is in master. |
That was my mistake. It is fixed in master but we may need to ask you to close and resubmit this PR. Thanks for the explanation. I should visit with my colleague who coded the original MATLAB version to confirm his reasons for rounding. |
|
@cedricleroy I talked with my colleague who originally wrote the MATLAB function and sorted through various discrepancies and typos in the two papers. We'll stick with [1] as the primary reference. For that reason I'd prefer to leave the factor as 0.163 rather than change to 0.16268. You are correct that the discrepancy between [1] and [2] is simply rounding to 3 digits. I don't know why that term in [1] is factored as it appears in [2], I'm guessing the authors of [2] have a physical interpretation of the leading constant 0.83 as the fraction extraterrestrial normal irradiance that arrives at the surface under clear sky conditions. We can certainly add a comment explaining the coefficients in the code and their source. Rather than carry this PR forward (since it has unrelated commits due to my error), if you'd like to suggest a comment to add, close this PR and open a new one. Or post your suggestion and I'll make a PR for it. |
|
Closing, superceded by #814 |
pvlib python pull request guidelines
Thank you for your contribution to pvlib python! You may delete all of these instructions except for the list below.
You may submit a pull request with your code at any stage of completion.
The following items must be addressed before the code can be merged. Please don't hesitate to ask for help if you're unsure of how to accomplish any of the items below:
docs/sphinx/source/api.rstfor API changes.docs/sphinx/source/whatsnewfile for all changes.Brief description of the problem and proposed solution (if not already fully described in the issue linked to above):
Maybe this gets too deep into the weeds, but opening a PR in case we want to do something about it.
I originally followed the SE 73 pp. 307-317 [2] paper when I noticed a small discrepancy in the
bterm due to what seemed like a rounding. After writing the fix, I realized that the other paper SE 73 pp. 151-157 [1] match perfectly with PVLib current implementation. 0.163 seems to be a rounding of 0.16268, so opening the PR anyway even if the fix is probably up to debate.We should perhaps include a comment explaining where it is coming from (similar to
bnci_2) asbcan be slightly different depending on which paper we are looking at.