[DRAFT] Allow arbitrary IAM function in pvlib.iam.marion_diffuse

[markcampanelli]

• Closes Allow arbitrary IAM function in pvlib.iam.marion_diffuse #2049
• 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.

TODOs:

• Add usage examples inline and/or in examples

[markcampanelli]

Before I work through the full checklist, I'm hoping a maintainer can do an early review of this to ensure it will be accepted.

I have added tests that also show how everything works, including how this adds support for using the existing pvlib.iam.interp with pvlib.iam.marion_diffuse. cc @adriesse

If this work should proceed, then I would be happy to add usage examples in separate scripts.

[cwhanse]

@markcampanelli code structure looks fine to me. I am not sure why adding iam.pchip fits in the scope. Accepting a callable doesn't require pchip. It looks to me that pchip is the equivalent of adding a new IAM model (defined by a specific interpolator). Also, what is the reason for needing the iam.pchip which is mostly a wrapper on the scipy function - is the scipy code awkward to use?

[markcampanelli]

@markcampanelli code structure looks fine to me. I am not sure why adding iam.pchip fits in the scope. Accepting a callable doesn't require pchip. It looks to me that pchip is the equivalent of adding a new IAM model (defined by a specific interpolator). Also, what is the reason for needing the iam.pchip which is mostly a wrapper on the scipy function - is the scipy code awkward to use?

@cwhanse Thanks much for taking an early look for purposes of direction and scope. In #2049, @adriesse suggesting mking pchip another option in the existing pvlib.iam.interp. I am ok doing that instead here (or removing pchip altogether). Thoughts?

In any event, I will keep the pchip function in the tests for test_marion_diffuse_iam_function_without_kwargs.

[markcampanelli]

@cwhanse @adriesse I decided simply to not add the PCHIP interpolator to the codebase, but it does appear as the main test case in test_marion_diffuse_iam_function_without_kwargs.

One issue I hit trying to integrate pchip into interp is that IAM functions in interp appear to NOT set IAM to zero for AOIs >= 90, and I'm am unsure what the justification is here. It also raised questions about the need to validate that IAM(0) > 0 and IAM(90) = 0.

I think my only remaining technical question is do I have to support the full numeric type for the aoi input in the callable accepted by marion_diffuse. I think the answer is yes.

[markcampanelli]

All this does is require that the n_ar argument be named, which seems like an antipattern for this codebase.

[cwhanse]

@markcampanelli a few in-flight comments:

• get_builtin_models should be private. I'm not sure of the advantage of this function over a constant `dict.
• I'm not sure why the code for fitting and converting models is being edited. That seems unnecessary as the PR isn't adding to the set of models that can be fit/converted.
• I'm OK with the ModelChain integration within this PR, but as @echedey-ls pointed out, sometimes with PRs less is more :)

[markcampanelli]

@cwhanse @kandersolar The sapm IAM function has a different interface from all the remaining IAM functions. I tried to clean this up in the code and make it much more obvious as to the where this inconsistency crops up. I also tried to clarify the handling of the required vs. optional parameters to the builtin IAM functions, after I sorted through all the various function interfaces (esp. adding interp with required and optional model parameters and adding schlick without any model parameters). I realize that this is adjacent to the feature added, but I had to understand how to fit in with this IAM code anyway, so I figured that I'd make the changes.

Question: Are folks open to switching the sapm function to the same interface as the other IAM functions, a breaking change?

Note that it has become too much to ask for the ModelChain initialization to infer the IAM function from the passed parameters in all cases. I did my best, and IMHO I think this inference should be removed altogether. (Better to just have the user be explicit.)

[markcampanelli]

@cwhanse

• get_builtin_models should be private. I'm not sure of the advantage of this function over a constant `dict.

Unfortunately, Python does not have true constant/immutable types, thus Python "constants" carry a risk that they will be mutated when used (often unintentionally, and causing horribly subtle bugs). By using a "getter" function instead, this mutation risk is greatly reduced. In this case, I did not make make it a private function because it gets used (like the pseudo-constant it supersedes) outside the module in which it is defined.

• I'm not sure why the code for fitting and converting models is being edited. That seems unnecessary as the PR isn't adding to the set of models that can be fit/converted.

The name _get_model really misdirected me at first. It cannot be used to get ANY of the models, but only the ones that are fittable/convertible. Since it's a private function, I took the opportunity to give it a more meaningful name that indicates its restricted scope. TBH: The scoping of things in this module was probably the hardest thing for me to grasp as a new contributor. If you don't know the scope of things, then you don't know what you might actually be touching/breaking.

• I'm OK with the ModelChain integration within this PR, but as @echedey-ls pointed out, sometimes with PRs less is more :)

Understood, but I have the present benefit of looking at this code with fresh eyes and seeing how hard it was to track down how different pieces are used in different parts, including outside this immediate Python module in the ModelChain. I also know that I don't really have the time to go back with a second PR, so I did it here. These are opportunities for what I call "harmonization", which I find many open source projects tend to miss (although, understandably so).

[cwhanse]

I would still make get_builtin_model private since it is hard to envisage its use outside the internal plumbing of pvlib. I'm sure it can find some use, but at this point it feels like clutter in the API. Thanks for explaining why the fit code is changing; makes sense to me now.

[cwhanse]

@markcampanelli I think the scope of this PR is becoming too broad. Can you break the PR into several smaller bites?

[markcampanelli]

@markcampanelli I think the scope of this PR is becoming too broad. Can you break the PR into several smaller bites?

@cwhanse Here is a proposal for two separate PRs:

1. Make the low-level SAPM effective-irradiance-related functions pvlib.iam.sapm and pvlib.spectrum.mismatch.spectral_factor_sapm consistent with their other respective iam and mismatch functions by taking each polynomial-coefficient parameter explicitly as a separate function parameter instead of the module dictionary that contains extra info. (This will not, however, break the higher level interfaces, such as PVSystem or ModelChain.) Update pvlb.modelchain.ModelChain.infer_aoi_model accordingly, using pvlib.iam._get_builtin_models. Rename pvlib.iam._get_model to pvlib.iam._get_fittable_or_convertable_model and update comments.
2. Convert the Marion model for diffuse effective irradiance to take an arbitrary IAM model function that meets the required interface that all "built-in" IAM models also meet (due to 1 above).

Thoughts?

[cwhanse]

Do we need to touch spectrum.mismatch.sapm to accomplish the end goal (allow any IAM model to be used with the Marion diffuse calculation)? If we don't need to change it, I would leave it of this initiative.

If we change both IAM and spectrum functions, I would divide #1 into two parts (separate iam and spectrum). We could get by without deprecating the old iam.sapm function since I doubt it gets used. But spectrum.mismatch.sapm probably does get used and should be deprecated until v0.12.

Overall I support the initiative here, to normalize the interfaces for these lower-level functions.