Add polo-smm #2491

jesuspolo · 2025-06-25T06:25:31Z

Add the polo spectral model for BIPV facades

Closes spectral model for BIPV in vertical facades #2406
I am familiar with the contributing guidelines
Tests added
Updates entries in docs/sphinx/source/reference for API changes.
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`).
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 (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

Add the polo spectral model for BIPV facades

AdamRJensen

@jesuspolo Thanks for opening your first PR!

I've made some formatting changes, to make sure that the code adheres to the standard flake8 conventions. If you go to the "File changed" tab of this PR, then you can click "Add suggestion to batch" one by one and then commit all of them at once.

pvlib/spectrum/mismatch.py

Co-authored-by: Adam R. Jensen <[email protected]>

AdamRJensen · 2025-06-25T08:43:40Z

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

jesuspolo · 2025-06-25T08:54:23Z

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)

pvlib/spectrum/mismatch.py

echedey-ls

Nice PR @jesuspolo !
There are a bunch of tasks to fulfil so this new feature becomes available to users (most of them are in the PR body):

Reference it in docs\sphinx\source\reference\effects_on_pv_system_output\spectrum.rst (so it appears in the documentation, once done, we will be able to review the built docs that appear down below)
Add a whatsnew entry in docs\sphinx\source\whatsnew
Add tests - the better the exhaustive and the most edge-cases they check

pvlib/spectrum/mismatch.py

Add test of use of spectral_factor_polo fuction (pull pvlib#2491)

RDaxini

Thank you for this contribution @jesuspolo
I just made a couple of small suggestions for changes to the docstring: rephrasing the summary line and linking the parameter terms to their respective definition on the nomenclature page.
You might already be working on this anyway, but I wanted to add that besides the test you already wrote, it would be good to add tests for edge cases like Echedey suggested. For example, how the function handles inputs such as NaN and 0, or how it handles inputs of different data types such as series or dataframe. I'm happy to help write and/or review such tests if you need, just ask.

pvlib/spectrum/mismatch.py

RDaxini · 2025-06-30T21:43:07Z

pvlib/spectrum/mismatch.py

+        * ``'monosi'`` - anonymous sc-si module.
+        * ``'cigs'`` - anonymous copper indium gallium selenide module.
+        * ``'asi'`` - anonymous amorphous silicon module.
+    albedo


I was more wondering if the code allows for albedo being passed in as a time series?

+1 to making this possible if not already

pvlib/spectrum/mismatch.py

echedey-ls · 2025-07-23T17:29:09Z

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

Co-authored-by: Echedey Luis <[email protected]>

AdamRJensen · 2025-10-25T16:17:27Z

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

jesuspolo · 2025-10-27T07:40:29Z

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

pvlib/spectrum/mismatch.py

adriesse · 2025-10-29T12:49:54Z

pvlib/spectrum/mismatch.py

+    altitude: numeric
+        altitude over sea level. [m]
+    module_type : str, optional
+        One of the following PV technology strings from [1]_:


Are the SR curves used to develop this model open data?

adriesse · 2025-10-29T13:02:47Z

pvlib/spectrum/mismatch.py

+    aod500 : numeric
+        atmospheric aerosol optical depth at 500 nm. [unitless]
+    aoi : numeric
+        Angle of incidence on the vertical surface.  See :term:`aoi`.


In the paper I read:

"...being the angle of incidence corresponding to 90° of tilt angle and the surface azimuth."

Does that really mean the same as this comment in the code?

It seems the same to me. What difference do you see @adriesse?

It's an awkward wording so I was just looking for confirmation from @jesuspolo

adriesse · 2025-10-29T13:09:46Z

pvlib/spectrum/mismatch.py

+    if module_type is not None and coefficients is not None:
+        raise ValueError('Only one of `module_type` and `coefficients` should '
+                         'be provided')
+    am_aoi = pvlib.atmosphere.get_relative_airmass(aoi)


This will produce a nan if aoi > 90.

@jesuspolo: how should $R_{AM}$ be calculated when the angle of incidence $\theta > 90\degree$ ? I guess something special should be done, but I do not see it specified in the paper.

kandersolar

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.

pvlib/spectrum/mismatch.py

jesuspolo · 2025-11-21T07:43:35Z

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 · 2025-11-21T07:49:13Z

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 · 2025-11-21T10:21:04Z

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.

adriesse · 2025-11-21T12:19:59Z

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.

jesuspolo · 2025-11-21T12:27:58Z

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.

adriesse · 2025-11-21T12:46:51Z

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.

jesuspolo · 2025-11-21T12:53:45Z

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?

cwhanse · 2025-11-21T15:11:15Z

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

jesuspolo · 2025-11-24T07:02:01Z

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

adriesse · 2025-11-24T12:57:33Z

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?

jesuspolo · 2025-11-24T17:33:56Z

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.

adriesse · 2025-11-25T11:22:26Z

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.

Add polo-smm

3363ace

Add the polo spectral model for BIPV facades

AdamRJensen added the enhancement label Jun 25, 2025

AdamRJensen reviewed Jun 25, 2025

View reviewed changes

Apply suggestions from code review

ceba758

Co-authored-by: Adam R. Jensen <[email protected]>

jesuspolo added 2 commits June 25, 2025 11:53

Update mismatch.py

36c9b43

Update mismatch.py

eaa90b4

AdamRJensen reviewed Jun 25, 2025

View reviewed changes

pvlib/spectrum/mismatch.py Outdated Show resolved Hide resolved

Update mismatch.py

794ed98

echedey-ls added this to the v0.13.1 milestone Jun 25, 2025

echedey-ls reviewed Jun 25, 2025

View reviewed changes

pvlib/spectrum/mismatch.py Outdated Show resolved Hide resolved

echedey-ls reviewed Jun 25, 2025

View reviewed changes

pvlib/spectrum/mismatch.py Show resolved Hide resolved

echedey-ls reviewed Jun 25, 2025

View reviewed changes

pvlib/spectrum/mismatch.py Outdated Show resolved Hide resolved

Update mismatch.py

7926a3d

echedey-ls reviewed Jun 25, 2025

View reviewed changes

pvlib/spectrum/mismatch.py Outdated Show resolved Hide resolved

jesuspolo added a commit to jesuspolo/pvlib-python that referenced this pull request Jun 27, 2025

Update test_mismatch.py

3493f79

Add test of use of spectral_factor_polo fuction (pull pvlib#2491)

RDaxini reviewed Jun 30, 2025

View reviewed changes

cwhanse reviewed Jun 30, 2025

View reviewed changes

pvlib/spectrum/mismatch.py Outdated Show resolved Hide resolved

pvlib/spectrum/mismatch.py Outdated Show resolved Hide resolved

pvlib/spectrum/mismatch.py Outdated Show resolved Hide resolved

pvlib/spectrum/mismatch.py Outdated Show resolved Hide resolved

Update mismatch.py

988557c

jesuspolo and others added 3 commits July 28, 2025 08:31

Update mismatch.py

45e768f

Update pvlib/spectrum/mismatch.py

38e5694

Co-authored-by: Echedey Luis <[email protected]>

Update pvlib/spectrum/mismatch.py

c24e013

Co-authored-by: Echedey Luis <[email protected]>

kandersolar modified the milestones: v0.13.1, v0.13.2 Sep 24, 2025

Changes from code review

0a00f1b

Add function to spectrum/__init__.py

2a9d0db

adriesse requested changes Oct 29, 2025

View reviewed changes

jesuspolo added 2 commits October 29, 2025 14:14

Update mismatch.py

31fe508

Update mismatch.py

c958abf

kandersolar reviewed Nov 19, 2025

View reviewed changes

kandersolar added 11 commits November 19, 2025 10:06

Apply suggestions from code review

bd1ff4f

replace altitude parameter with pressure

f0a4f34

albedo : numeric

ee7f13f

make albedo coefficients user-specified as well

82598ec

complete tests

8b2b7ca

docstring cleanup

31f26b5

Merge remote-tracking branch 'upstream/main' into pr/2491

f4e07de

whatsnew note and contributors

5ec4bee

add to user guide SMM model comparison table

d0efc37

lint

297f160

minor doc fixes

c9b9816

Update mismatch.py

3df13ae

Update mismatch.py

3015b3a

kandersolar mentioned this pull request Dec 1, 2025

v0.13.2 planning #2604

Open

5 tasks

Add polo-smm #2491

Are you sure you want to change the base?

Add polo-smm #2491

Uh oh!

Conversation

jesuspolo commented Jun 25, 2025 • edited by AdamRJensen Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AdamRJensen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AdamRJensen commented Jun 25, 2025

Uh oh!

jesuspolo commented Jun 25, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

echedey-ls left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RDaxini left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

RDaxini Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

echedey-ls commented Jul 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AdamRJensen commented Oct 25, 2025

Uh oh!

jesuspolo commented Oct 27, 2025

Uh oh!

Uh oh!

adriesse Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

adriesse Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

kandersolar Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

adriesse Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

adriesse Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

kandersolar Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

kandersolar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jesuspolo commented Jun 25, 2025 •

edited by AdamRJensen

Loading

RDaxini left a comment •

edited

Loading

echedey-ls commented Jul 23, 2025 •

edited

Loading