Implement Caballero et al. spectral factor model #1296
Conversation
|
Hi @Jacc0027 -- sorry for the delay if you were waiting for feedback on this PR! It looks great so far. To make a more detailed review easier, could you address the code style issues noted by "Stickler CI" on this page? https://github.com/pvlib/pvlib-python/pull/1296/files This will also need some tests (see the contributing docs) to check that the code works on all the various python versions and package versions we support. Please reach out if you need input from us to move this PR forward :) And thanks again for the contribution! |
…lib-python into Spectral-corrections
Hi @kanderso-nrel , thank you for your help. After a bit of time and testing, I finally managed to adapt the code and pass the Stickler CI test. It is a pleasure for me to contribute to this wonderful project. I remain at your disposal for any further issues. |
|
Thanks @Jacc0027 for the effort here! I think I see how to get the tests working; would you mind if I pushed some commits to this PR for that? Then after a bit of documentation I think this PR will be ready. |
Hi @kandersolar , no problem. Go ahead with the solution. Thanks for the support! |
|
Looks like things are working! I think these are the last main things, @Jacc0027 can you take a look:
|
Updated both "TODO" issues.
Thanks @kandersolar you did it!. I have done the 3 things you've requested me. Please, let me know if you need something else. |
|
Hmm, for some reason this PR is not showing any changes to the Maybe you only changed them locally but did not include them in a commit? |
I requested to update both files after modifying them via Github online. After requesting the Commit of the changes, I got an option to create a PR for them. Should I create these new PRs? |
|
It would be better to include them in this PR so that all the changes are in one place. If you are editing them with Github online, then I think you can add the changes to this PR by editing the files on the specific git branch associated with this PR (the If you edit that file and go to commit the change, it should give you the option "Commit directly to the |
pvlib/atmosphere.py
Outdated
| as a function of AM, AOD and PW was performed according to [2] and [3]. | ||
| As such, the polynomial adjustment coefficients included in [3] | ||
| were obtained. | ||
|
|
There was a problem hiding this comment.
I think it is very good to have a couple of paragraphs explaining the basics here, but unfortunately I find them hard to understand. If you recorded AM, AOD, PW and spectra, then where does smarts come in? I guess I should read the paper, but this text should also be consistent and comprehensible on its own.
There was a problem hiding this comment.
Hi @adriesse, the reason for using synthetic spectra, generated through SMARTS2, was to ascertain the ideal polynomial type of the experimental equations related to the AM, AOD and PW parameters.
pvlib/atmosphere.py
Outdated
| Spain for one year synchronously with both, broadband and | ||
| spectroradiometric measurements of 30º tilted global irradiance | ||
| south-facing logged in 5-min intervals. AM was estimated through | ||
| elevation data. |
There was a problem hiding this comment.
Does this estimation refer to solar elevation? Airmass calculations are pretty standard.
There was a problem hiding this comment.
Hi @adriesse, yes it does. The solar elevation was measured by means of a solar spectral irradiance meter (SolarSIM-D2) from Spectrafy Inc. Then the AM was computed by using the sun’s zenith angle (z). Reference: F. Kasten and A. T. Young, “Revised optical air mass tables and approxi-mation formula,” Appl. Opt., vol. 28, pp. 4735–4738, Nov. 15, 1989.
There was a problem hiding this comment.
My thinking was that your responses might make it into the doc string at some point...
Added caballero spectral correction as a new function.
Added a new function: atmosphere.caballero_spectral_correction
Hi @kandersolar, done. Thank you for the guidelines! |
Co-authored-by: Anton Driesse <anton.driesse@pvperformancelabs.com>
following the conventions set out in pvlib#1628
- reorder and rename parameters to match pvlib#1768 - remove reference parameters - shorten/simplify docstring - restructure calculation to be more readable
kandersolar
changed the title
|
Hi @Jacc0027, I just added a few changes. The main one is to move the new function to I think this PR is ready to merge, but I'll leave it open for a few days to allow a chance for anyone else to review again. |
Thank you @kandersolar .
Hi @kandersolar, thank you so much for the effort! There have been two major solar energy events during the last week: Intersolar Europe 2023 and PVSC50. Perhaps during the week ahead there will be some additional comments. We'll see. |
| f_AM = ( | ||
| coeff[0] | ||
| + coeff[1] * ama | ||
| + coeff[2] * ama**2 | ||
| + coeff[3] * ama**3 | ||
| + coeff[4] * ama**4 | ||
| ) | ||
| # Eq 6, with Table 1 | ||
| f_AOD = (aod500 - aod500_ref) * ( | ||
| coeff[5] | ||
| + coeff[10] * coeff[6] * ama | ||
| + coeff[11] * coeff[6] * np.log(ama) | ||
| + coeff[7] * ama**2 | ||
| ) | ||
| # Eq 7, with Table 1 | ||
| f_PW = (precipitable_water - pw_ref) * ( | ||
| coeff[8] | ||
| + coeff[9] * np.log(ama) | ||
| ) |
There was a problem hiding this comment.
There are a several options for making this more efficient: Horner's method, np.polyval, pre-calculating ama powers, expanding **3 and **4, adding coefficients before multiplying where applicable. Just thought I'd mention that since it came up in other places in the past. They would all reduce the much-appreciated code clarity to varying degrees, so I'll stop short of making a recommendation.
There was a problem hiding this comment.
I felt the same conflict between code clarity and efficiency. I settled in favor of clarity after seeing that this inefficient implementation is still plenty fast by my standards (computes ~10 million values in one second).
Still, it does offend the sensibilities to calculate x**3 on one line and x**4 on the next :(
|
Thanks again @Jacc0027 for this contribution, and my apologies that it took nearly two years to get it merged. Better late than never, hopefully :) |
|
Thank you so much @kandersolar, @adriesse, @cwhanse, @mikofski for your support during this time! To a lesser or greater extent, the closing of this contribution has also been possible thanks to your assistance. I would really wish I could have made this contribution in other circumstances with more time available. |
- Closes Add a model for spectral corrections #1278
- I am familiar with the contributing guidelines
- Tests added
- Updates entries to
- Adds description and name entries in the appropriate "what's new" file in
- 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.
https://github.com//issues/1278docs/sphinx/source/api.rstfor API changes.docs/sphinx/source/whatsnewfor 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`).AM_AOD_PW_spectral_correction.zip