pvlib-python icon indicating copy to clipboard operation
pvlib-python copied to clipboard

Add polo-smm

Open jesuspolo opened this issue 6 months ago • 18 comments

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/reference for API changes.
  • [x] Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for 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 avatar Jun 25 '25 06:06 jesuspolo

@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.

AdamRJensen avatar Jun 25 '25 08:06 AdamRJensen

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 avatar Jun 25 '25 08:06 jesuspolo

@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.

echedey-ls avatar Jul 23 '25 17:07 echedey-ls

@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.

AdamRJensen avatar Oct 25 '25 16:10 AdamRJensen

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

jesuspolo avatar Oct 27 '25 07:10 jesuspolo

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

jesuspolo avatar Nov 21 '25 07:11 jesuspolo

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

jesuspolo avatar Nov 21 '25 07:11 jesuspolo

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.

jesuspolo avatar Nov 21 '25 10:11 jesuspolo

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.

adriesse avatar Nov 21 '25 12:11 adriesse

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.

jesuspolo avatar Nov 21 '25 12:11 jesuspolo

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.

adriesse avatar Nov 21 '25 12:11 adriesse

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 avatar Nov 21 '25 12:11 jesuspolo

@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.

cwhanse avatar Nov 21 '25 15:11 cwhanse

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

jesuspolo avatar Nov 24 '25 07:11 jesuspolo

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?

adriesse avatar Nov 24 '25 12:11 adriesse

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.

jesuspolo avatar Nov 24 '25 17:11 jesuspolo

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.

adriesse avatar Nov 25 '25 11:11 adriesse

I think all code changes identified in review have now been addressed. I'll plan to merge this PR tomorrow unless objections are voiced.

kandersolar avatar Dec 15 '25 19:12 kandersolar

Many thanks to all reviewers here, and to @jesuspolo for sticking with the process!

kandersolar avatar Dec 16 '25 14:12 kandersolar

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)

jesuspolo avatar Dec 16 '25 15:12 jesuspolo