Add polo-smm
Add the polo spectral model for BIPV facades
- [x] Closes #2406
- [X] I am familiar with the contributing guidelines
- [x] Tests added
- [x] Updates entries in
docs/sphinx/source/referencefor API changes. - [x] Adds description and name entries in the appropriate "what's new" file in
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`). - [x] New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
- [x] Pull request is nearly complete and ready for detailed review.
- [x] Maintainer: Appropriate GitHub Labels (including
remote-data) and Milestone are assigned to the Pull Request and linked Issue.
Adding the spectral model for estimating spectral mismatch for BIPV in facades according to ref: Polo, J., Sanz-saiz, C., 2025. Development of spectral mismatch models for BIPV applications in building façades Abbreviations : Renew. Energy 245, 122820. https://doi.org/10.1016/j.renene.2025.122820
@jesuspolo As you can see, the Python Flake8 linter action (shown at the bottom of this PR) failed, due to some formatting not adhering to the Flake8 standard (I guess I didn't manage to spot all of them). If you click the action you can see what still needs to be fixed.
Whenever you make changes online and you switch to working on GitHub Desktop, you need to remember to click the "Fetch origin" button to make sure you have the latest changes locally.
Hi Adam, I don't know how to solve the problems, I've seen they are about line too long and spaces, but I don't know how to edit the code and solve (sorry for my few capabilities)
@jesuspolo I'm wondering about your current status with this PR. I see that some of the required tasks I reminded in this comment haven't been fulfilled. If you want, you can contact me at echedey.luis.alvarez (at) upm . es for support in Spanish.
@jesuspolo I cleaned up a few things and implemented the suggested changes above (they are now shown as resolved).
First, could you comment on whether the albedo could be made a time series, such that the albedo could be changing throughout the year?
Second, test functions need to be added here. Tests are functions that tests that specific parts of the code works as expected and don't suddenly break when we add some other code. I suggest that you look at the test for the existing "spectral_factor_" functions for inspiration.
As always, feel free to reach out here or via email.
Albedo, as occurs with AOD, PW, airmass can input as numpy array . Thus the function allow to compute changes in albedo along the time, as well as chages in aod or in precipitable water
I think this PR is getting close, but several things identified previously still need to be completed, including:
- more comprehensive tests (nan, zero, aoi>90, etc)
- whatsnew entry
- misc cleanup
The suggestions below are a start, but I'll add a few additional commits to get this PR ready for final review/merge.
Hi Kevin, thanks so much for your help in this PR; as you can see it is confuse to me to work on Github and I don't know usually if I have corrected or fixed things properly. I have noticed your last comment regarding AOI>90; thanks a lot for this interesting comment. Honestly I did not think about this posibility; the model is not prepared for that case, so I should do something to prevent this situation, otherwise the smm is nan. So the model is limited to AOI below 90 degress (this is a limitation, sue to how this model has been formulated, because it makes use of karsten formula of air mass with the AOI.). ANy idea on what we must do (a print message or whatever) will be welcome.
Please, tell me if I have to do something to implement your changes or if they are already implemented. I normally make all the correction in my filder and then use Gihub desktop to pull , fetch and run...
Thanks again to all you a lot for guiding me
Jesus
I think this PR is getting close, but several things identified previously still need to be completed, including:
- more comprehensive tests (nan, zero, aoi>90, etc)
- whatsnew entry
- misc cleanup
The suggestions below are a start, but I'll add a few additional commits to get this PR ready for final review/merge.
Hi Kevin, thanks so much for your help in this PR; as you can see it is confuse to me to work on Github and I don't know usually if I have corrected or fixed things properly. I have noticed your last comment regarding AOI>90; thanks a lot for this interesting comment. Honestly I did not think about this posibility; the model is not prepared for that case, so I should do something to prevent this situation, otherwise the smm is nan. So the model is limited to AOI below 90 degress (this is a limitation, sue to how this model has been formulated, because it makes use of karsten formula of air mass with the AOI.). Any idea on what we must do (a print message or whatever) will be welcome.
Clearly AOI >90 means that the surface is not iluminated so, but to prevent any undesirebale situation maybe I could force smm to be 1 in case of not ilumination (aoi >90) . Is this a plausible approac?
Please, tell me if I have to do something to implement your changes or if they are already implemented. I normally make all the correction in my filder and then use Gihub desktop to pull , fetch and run...
Thanks again to all you a lot for guiding me
Jesus
I have add an if to prevent nan in case of aoi>90, then the function output is 1, I assume that no spectral correction is needed in that case.
Clearly AOI >90 means that the surface is not illuminated so, but to prevent any undesirable situation maybe I could force smm to be 1 in case of not illumination (aoi >90) . Is this a plausible approach?
I don't think so because you will often still have diffuse irradiance. This logic could be used for zenith>90.
It is true Anton, you are right (good point!!), but for practical situations, should be aware of spectral mismatch when the surface is only illuminated by diffuse irradiance? Oviously, this is a limitation of the model. For instance, the procedure for testing and finding the parameters of the King, First-solar, Caballero, and so on spectral models were tested also with only pure diffuse irradiance? Anyway, if you find another solution for the situation of AOI>90 we can included. It is clear that the model was developed only for AOI<90.
Well, here we have to implement what was published, but I would say as AOI approaches 90, the irradiance is mostly diffuse, so probably the best SMM value to use for AOI>90 is the SMM value at (or just before) AOI=90.
Thanks so much Anton, this is a good recommendation, it makes a lot of sense. Should be this recommendation be as a note in the information and comments of the function? SHould we keep (for safety reasons) the forcing ssm to 1 for aoi >90 to prevent nan?
@jesuspolo the pvlib function needs to return something for aoi>90. If you (as primary author of the reference) think that returning 1 is reasonable (I agree with @adriesse here), please add an inline comment to that effect.
I have introduced an if condition forcing AOI to 90 (in case of AOI>90); I think that Anton suggestion has physical meaning and is a good way to solve the problem, and it gives continuity to the expected response of the function as AOI is increasing. Thanks for all the comments and support
Before we jump to a conclusion @jesuspolo can you provide some information about how points with AOI>90 were treated in your model development and validation?
The model was developed using only SMM values computed for AOI lower than 90, becasue I used the Kasten formulae for zenith angle with the AOI. SO in theory the model is limited to AOI lower than 90. To me the imposition of OAI to 90 sounds logical, but other option is just to print a message and givin no results when the AOI is greater.
The model was developed using only SMM values computed for AOI lower than 90
Ok, I think this information should be included in the docstring somewhere. The docstring should also explain that the pvlib implementation is extended for practical purposes (and how). A comment in the code is not enough, I believe.
I think all code changes identified in review have now been addressed. I'll plan to merge this PR tomorrow unless objections are voiced.
Many thanks to all reviewers here, and to @jesuspolo for sticking with the process!
Thanks a lot Kevin for your help and support; thanks so much also to Adam, Anton and Cliff for the comments, suggestions and help. Thanks to all the community for letting me to participate in this useful project (pvlib)