Inappropriate module name for "pvlib.iam.schlick_diffuse"

[xieyupku]

Describe the bug

I am one of the major developers of the "Fresnel Equations" for Diffuse radiation on Inclined photovoltaic Surfaces (FEDIS) which is referred to, in pvlib, as pvlib.iam.schlick_diffuse. First of all, thank you for including FEDIS in pvlib. However, as we discussed in the PVSC meeting, it is an apparent mistake to use the name "Schlick diffuse" in pvlib. Schlick model is only a simple approximation of the Fresnel equations for direct radiation. It is much complicated to give the precise iam solution for diffuse radiation due to the integration in space. FEDIS derived an analytical solution of the iam model for diffuse radiation, with limited and clearly stated approximation, using the Schlick equation for direct radiation. The solution given by FEDIS is completely different with Schlick equation, however, and was not provided in Schlick paper. It is thus very misleading and essentially wrong to use the current name to describe FEDIS model.

Therefore, I strongly suggest that the module name should be changed to pvlib.iam.fedis or pvlib.iam.fedis_diffuse.

Thank you for considering my comments.
Yu Xie

To Reproduce
Steps to reproduce the behavior:

1. Go to '...'
2. Click on '....'
3. Scroll down to '....'
4. See error

Expected behavior
I strongly suggest that the module name should be changed to pvlib.iam.fedis or pvlib.iam.fedis_diffuse.

Screenshots
If applicable, add screenshots to help explain your problem.

Versions:

• pvlib.__version__: 0.10.1
• pandas.__version__:
• python:

Additional context
Add any other context about the problem here.

[kandersolar]

Hello @xieyupku, thanks for opening this issue. If there is a mistake in naming that function, the mistake is mainly mine.

The motivation for the name "schlick_diffuse" is that it is the diffuse IAM model that follows from the Schlick direct IAM model. It was intended to reflect the theoretical basis of the model, not its publication origin. It is beneficial for pvlib.iam.schlick and pvlib.iam.schlick_diffuse to have similar names so that the mathematical relationship is clear. Hypothetically, if someone developed a diffuse model based on the ASHRAE direct model, I think it would be reasonable to call it "ashrae_diffuse", even if the diffuse model was not directly associated with ASHRAE itself. Another example already in pvlib is faiman_rad, which is an extended form of the faiman model. "faiman_rad" was not developed by Faiman, but we still include the name to emphasize the conceptual connection with the original model. So to me, the name "schlick_diffuse" makes sense and I am not convinced it would be better to change it to anything else.

Additionally, I don't think the name "fedis_diffuse" would be correct. When we were implementing the code, we came to the conclusion that, in our view, the refractive index weighting coefficient $w$ in the FEDIS model is not theoretically justifiable. Because of that, we did not want pvlib to include any model with the $w$ term. However, we decided that the analytical integration of the Schlick equation was suitable to include on its own, so we implemented only that part while leaving out the $w$ term. Since that function does not have $w$, it does not implement the FEDIS model, so it would be incorrect to call it "fedis_diffuse".

However, I see the point that some people might interpret the name as suggesting that the analytical integration was given by Schlick. To try to make it clear where the model came from and give credit to the FEDIS authors, of course we were sure to cite the FEDIS paper in the function's documentation (link) and include comments in the code indicating the origin of each equation (link). So readers should understand that the source of the analytical integration is the FEDIS paper, but any ideas to improve the documentation so that this is clearer are very welcome. Maybe we should expand the description in the documentation of "schlick_diffuse" to be more explicit about where the model came from?

[cwhanse]

I agree with @kandersolar on this issue.

Maybe we should expand the description in the documentation of "schlick_diffuse" to be more explicit about where the model came from?

We can add a :math: expression for the Schlick approximation for direct IAM, and describe that Yu et al. provide the integration of that expression to obtain an IAM model for isotropic sky diffuse irradiance.

[xieyupku]

@kandersolar @cwhanse Thank you so much for specifying more details and your concern on choosing the module name. I agree that it is very important to let users know the correct origin and theoretical basis of the model. For this module, pvlib.iam.schlick_diffuse, the theoretical basis is the Fresnel equations, however, not the Schlick equation. The Schlick equation itself does not have any physical meaning. Instead, it is only a mathematical approximation of the Fresnel equations. Because of that, I feel it would be much more appropriate if the module name is Fresnel_diffuse, which is actually very similar as the real name, the "Fresnel equations" for diffuse radiation over on inclined PV Surfaces (FEDIS).

I understand you want to follow deeper to the origin of the equations. However, this is usually not the case in the scientific field. For example, the famous Newton's law of universal gravitation was derived from the Kepler's law. But it is incorrect to refer it to something related to Kepler's law due to the further derivation and investigation. For FEDIS, we only used Schlick equation as a simple tool and a starting point. The theoretical basis of FEDIS is the Fresnel equations. The final form of the equations and coefficients actually have no much relation with the Schlick equation. Because of that, we believe it is incorrect to use the current name. Please note that this is not just my opinion. All the developers and sponsor of FEDIS are very surprised and unhappy to see this inappropriate name of the module when used by pvlib.

Also, the FEDIS equations were not approved by Schlick. I am not sure if we might have legal issues to use the name, Schlick diffuse.

As for the refractive index issue, FEDIS is more flexible to use different refractive index. A hard coded refractive index in the equations in pvlib can be considered as a special case of FEDIS. So we believe it is still appropriate to use FEDIS to represent the module.

Those are our opinions as the module developers. Thank you for your consideration!

[cwhanse]

@xieyupku Eq. 4 in your paper is an integration of Schlick's approximation of the Fresnel equation. It is not an integration of the Fresnel equation. Hence the name is not fresnel_diffuse.

I think too much is being read into the use of the name schlick.

[xieyupku]

@cwhanse @kandersolar Thanks for joining in the discussion. I understand it is too much discussion for a small module included in pvlib, but it took the developers much longer time to build the model. For the benefit of all the developer, I am just expressing the opinion on behalf of them. Of course, it is totally up to you to make the final decision.

To be short, I think the current name is fine if we numerically solve the integration of Schlick model by just using Eq.(A4 and A5). If you look at the derivation of the analytical solution in the Appendix, Eq.(4) has no much relation with Schlick equation. The solution was not given by Schlick, nor approved by Schlick. The current name is not favorable by the developers, the sponsor, and Schlick. I think it would be the best that we simply replace the name as so many people do not like it.

[cwhanse]

If you look at the derivation of the analytical solution in the Appendix, Eq.(4) has no much relation with Schlick equation.

Eq. 4 in the paper is Eq. A10. Eq. A10 is the result of evaluating the integral in Eq. A8a, which is the integration of Schlick's formula (stated in Eq. A7). So Eq. 4 is the integration of Schlick, unless I am missing something major in your paper.

[xieyupku]

@cwhanse @kandersolar As you can see in the paper, 99% of the efforts in developing FEDIS is to analytically solve the integration. Schlick equation is a very small part to start with. As I mentioned, you use Kepler's law to derive Newton's law of universal gravitation, the solution is not Kepler's law anymore. However, I leave the final decision to the pvlib team.

[kandersolar]

The model in question does not use the Fresnel equations, so we cannot name it "fresnel_diffuse". Neither can we name it "fedis_diffuse", since it is not the FEDIS model. The model is precisely the diffuse form of the Schlick equation, so "schlick_diffuse" is the correct name for it. The function name reflects what the model is, not where it came from. I think we will not be convinced that the function name should be changed.

I have submitted a proposed change to the function's documentation in #1812 to clarify the origin of the integration and the function's relationship with the FEDIS model. A preview of the updated documentation is available here: https://pvlib-python--1812.org.readthedocs.build/en/1812/reference/generated/pvlib.iam.schlick_diffuse.html

Comments on this proposed change are welcome.

[kandersolar]

Hearing no objections, I will go ahead and merge #1812. I hope the new documentation is sufficiently clear in exactly what the pvlib function implements, and what the original sources of the equations are.

The clarifications will be included in the documentation for v0.10.2 when it is released. Until then, it will be in the "development" documentation at https://pvlib-python.readthedocs.io/en/latest/.